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

[JENKINS-49377] Stop reading or writing BsiToken #60

Merged
merged 2 commits into from Feb 9, 2018

Conversation

jglick
Copy link
Member

@jglick jglick commented Feb 6, 2018

JENKINS-49377

Would be better if the parent POM were updated to something newer, and then Jenkinsfile could

buildPlugin(jenkinsVersions: [null, '2.104'])

and have a JenkinsRule-based test which would do configRoundtrip on StaticAssessmentBuildStep, as that would most likely reproduce the error with the src/main/ changes temporarily reverted.

@reviewbybees esp. @oleg-nenashev

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

@@ -0,0 +1,2 @@
# for compatibility with old JobModel instances which still saved the bsiToken field:
com.fortify.fod.parser.BsiToken
Copy link
Member

Choose a reason for hiding this comment

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

probably not required if you rename the field

Copy link
Member Author

Choose a reason for hiding this comment

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

Depends on whether you want to maintain compatibility with existing settings or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I think you are right—both bsiToken and bsiTokenOriginal seem to have been added at the same time in #47 (which BTW offered no readResolve), so any instance with bsiToken should also bsiTokenOriginal.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

🐝 , I'd guess

@mtgibbs
Copy link
Contributor

mtgibbs commented Feb 6, 2018

What's the concern here? If you don't save the token, the users settings are lost. I'm confused by the description of this PR.

@jglick
Copy link
Member Author

jglick commented Feb 8, 2018

If you don't save the token, the users settings are lost.

No they are not, because this plugin was already saving the String bsiTokenOriginal. It was just also saving this weird data structure with the same information (as of #47).

…suffice to rename bsiToken so we never even try to deserialize a BsiToken instance.
@jglick jglick changed the title [JENKINS-49377] Whitelist BsiToken but stop saving it going forward [JENKINS-49377] Stop reading or writing BsiToken Feb 8, 2018
@oleg-nenashev
Copy link
Member

PR build failed due to the INFRA issue from what I see.
CC @olblak

@olblak
Copy link
Member

olblak commented Feb 9, 2018

@oleg-nenashev Indeed ci.j.io had disk issue yesterday, it's working now

@mtgibbs
Copy link
Contributor

mtgibbs commented Feb 9, 2018

Alright, I see the issue now. Thanks for the change on this to support 2.x. I'll get some testing done on this to make sure people don't have upgrade issues then publish with this fix in it.

@mtgibbs mtgibbs merged commit b234fe9 into jenkinsci:master Feb 9, 2018
@oleg-nenashev
Copy link
Member

@mtgibbs Let me know once the fix is released. I will update https://wiki.jenkins.io/display/JENKINS/Plugins+affected+by+fix+for+JEP-200

@jglick jglick deleted the JENKINS-49377 branch February 12, 2018 15:56
@oleg-nenashev
Copy link
Member

@mtgibbs gentler ping.

@oleg-nenashev
Copy link
Member

@mtgibbs Jenkins LTS 2.107.1 will be released on March 14. Starting from this date we expects a significant number of users to start upgrading to the Jenkins versions with JEP-200

@mtgibbs
Copy link
Contributor

mtgibbs commented Mar 5, 2018

Yeah, we just got our FoD release out last week, so this week I should be able to get this tested and published.

@mtgibbs
Copy link
Contributor

mtgibbs commented Mar 14, 2018

@oleg-nenashev I've published @jglick 's fix today. You should be able to update the ticket. 3.0.8 should be the version containing the change.

@oleg-nenashev
Copy link
Member

@mtgibbs sorry for the response delay. Thanks a lot for the release!

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.

None yet

4 participants