Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: When configured using a pipeline, handle null filterable #85

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

rahulsom
Copy link

@rahulsom rahulsom commented Aug 2, 2023

When using pipelines, people tend to skip filterable as a parameter. That causes filterable to be set to null. Stapler would emit an empty string.

The prior behavior used javascript truthiness and treated it as false.

Since moving almost all javascript into separate files, the new behavior causes an empty string to be printed during the method call.

This change guarantees we emit a primitive boolean instead of a boxed Boolean.

Refs: JENKINS-71724

Testing done

Added new tests.
Existing tests pass.

Submitter checklist

Preview Give feedback

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @rahulsom ! I saw some discussion on the issues, but I am not able to follow that at the moment.

Build is failing, but the code/tests look good to me. Once the build is green I can merge/cut a release.

Thanks!

When using pipelines, people tend to skip `filterable` as a parameter. That causes `filterable` to be set to `null`. Stapler would emit an empty string.

The prior behavior used javascript truthiness and treated it as `false`.

Since moving almost all javascript into separate files, the new behavior causes an empty string to be printed during the method call.

This change guarantees we emit a primitive `boolean` instead of a boxed `Boolean`.

Refs: JENKINS-71724
@rahulsom
Copy link
Author

rahulsom commented Aug 2, 2023

It's failing to checkout - I've started a discussion - https://groups.google.com/g/jenkinsci-dev/c/JlSMkucHulo

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@kinow
Copy link
Member

kinow commented Aug 2, 2023

Shall we merge and release this, @rahulsom ?

@rahulsom
Copy link
Author

rahulsom commented Aug 3, 2023

Yep. We're good to release this. Thanks!

@kinow kinow merged commit d6247d2 into jenkinsci:master Aug 3, 2023
kinow added a commit that referenced this pull request Aug 3, 2023
@kinow
Copy link
Member

kinow commented Aug 3, 2023

Release started, then stopped. All tests worked, Maven build was going fine, then Jenkins maven repo rejected my credentials.

No changes on my side since last release a few weeks ago (as I'm working more on Python nowadays). Checked Maven security settings and they look good.

Logged out, then logged in again in JIRA, all good. But https://repo.jenkins-ci.org/ui/login/ rejects my credentials. Tried to recover password, and it says my user either doesn't exist or is locked.

One of the reasons I stopped contributing to Jenkins projects & plugins was the amount of things needed to stay atop of the moving parts, and the time spent troubleshooting issues like this ().

I have a full day with things I will have to deliver at $work, then a medical appointment at evening. Ditto for tomorrow. Will try the mvn release:perform again in the evening or weekend, but if that doesn't work in the next days I might have to leave it like that until I'm back from Brazil by the end of September.

@imoutsatsos
Copy link
Member

imoutsatsos commented Aug 3, 2023

I understand how frustrating things like that are @kinow . Many hours of my life have been spent battling stupid security issues. I hope maven is good to you soon! I have tried the link on JFrog and it seemed to work with my Jenkins JIRA credentials.

@oleg-z
Copy link

oleg-z commented Aug 3, 2023

Logged out, then logged in again in JIRA, all good. But https://repo.jenkins-ci.org/ui/login/ rejects my credentials. Tried to recover password, and it says my user either doesn't exist or is locked.

Can someone from jenkins core team help with the issue? @oleg-nenashev @jglick?

@jglick
Copy link
Member

jglick commented Aug 3, 2023

@jglick
Copy link
Member

jglick commented Aug 3, 2023

(or switch to https://www.jenkins.io/doc/developer/publishing/releasing-cd/ in which case you never need to bother with personal Artifactory credentials again)

@rahulsom rahulsom deleted the JENKINS-71724 branch August 5, 2023 22:46
@eplodn
Copy link

eplodn commented Aug 16, 2023

@kinow Nothing has been released to include this fix as of now, correct?

@kinow
Copy link
Member

kinow commented Aug 16, 2023

That's correct @eplodn. After my last message I tried to perform the last step of the release process one last time, and the account was probably still locked or something (last time I used the credentials was for the previous release 🤷 ).

I'm busy with last minute deliveries at $work before going to visit family. I'll be back by October, and then once I find spare time I will try the jenkins-infra/helpdesk issues or switching as Jesse suggested. If the issue or switch do not take too long, a new release should be out somewhere in October.

Sorry.

@basil
Copy link
Member

basil commented Aug 17, 2023

Released in 2.7.2.

@kinow
Copy link
Member

kinow commented Oct 11, 2023

(or switch to https://www.jenkins.io/doc/developer/publishing/releasing-cd/ in which case you never need to bother with personal Artifactory credentials again)

I had saved it to read later, and I had completely missed any news about this release process. It looks a lot simpler, but then I have to remember to use a separate branch and follow all those steps.

What did help me, though, was the link in that page to the docs to release manuallly. There was a curl command there, right at the beginning.

I executed that command, updated my settings.xml with the returned text, and everything worked smoothly. Thanks! cc @imoutsatsos in case that ever happens or you need some help releasing the plug-in and I cannot help, have a look at these two links ☝️

@jglick
Copy link
Member

jglick commented Oct 11, 2023

I have to remember to use a separate branch

Sorry, what do you mean by a separate branch? Maybe there is something that needs to be clarified in the docs.

@kinow
Copy link
Member

kinow commented Oct 11, 2023

I have to remember to use a separate branch

Sorry, what do you mean by a separate branch? Maybe there is something that needs to be clarified in the docs.

Hi @jglick , maybe I misunderstood this part

Push the work to prepare your plugin for CD to a dedicated branch, which you file a pull request from to your default branch. When enabling CD, the hosting team reviews this PR containing all necessary changes in your plugin, to prevent improper releases and mistakes. Don’t push the changes directly to the default branch or merge the PR before the hosting team has reviewed it.

I thought it was suggesting to use something like release or develop branches, and that pull requests would have to be made to this branch, and eventually merged into master or main to be automagically released (like in gitflow).

@jglick
Copy link
Member

jglick commented Oct 12, 2023

No no, not at all. The “dedicated branch” refers to setting up the plugin for CD, one time. This is just saying that when following the instructions in the guide, rather than pushing all those changes straight to master, it is better to create a regular PR that the hosting team can review in case you missed a step or whatever. Then once cd.enabled is merged to trunk in RPU and the Artifactory secrets appear in your repo, you are all set to merge the CD setup PR. After that, you just merge PRs to your plugin’s default branch (e.g., master) normally, taking care to apply a label like bug or chore both for release notes categorization and to instruct the CD system whether a release is needed or not.

@kinow
Copy link
Member

kinow commented Oct 12, 2023

Aaahh! That doesn't sound too bad! I might try that in one of the next releases (and if that works will move other plugins to CD too). Thanks for clarifying @jglick !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants