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

Simplify Jenkins.installState serial form to avoid relying on readResolve #3405

Merged
merged 2 commits into from May 3, 2018

Conversation

2 participants
@jglick
Member

jglick commented Apr 24, 2018

Follows up #3325. After that was merged I had on occasion noticed warnings from readResolve that valueOf failed to find a state by name, which I am presuming was caused by the extension list not being ready at the time of deserialization. I am not sure how to reproduce those problems, but switching the serialized field to a plain String and then looking up the live InstallState object lazily once the system is in a readier state seems safer, and also simplifies the $JENKINS_HOME/config.xml form to just, e.g.,

<installStateName>RUNNING</installStateName>

Proposed changelog entry: further simplify storage of the Jenkins setup wizard’s installation state.

@jenkinsci/code-reviewers

@jglick jglick requested a review from Vlatombe Apr 24, 2018

@Vlatombe

This comment has been minimized.

Member

Vlatombe commented Apr 24, 2018

Will break the proprietary RegistrationState.

@jglick

This comment has been minimized.

Member

jglick commented Apr 24, 2018

@Vlatombe IIUC there is a subclass overriding readResolve and including state. It should not be doing so—install states are intended to be singletons, as documented in the super readResolve. Instead, if there is any interesting state, this may be kept in a separate GlobalConfiguration.

@Vlatombe

lgtm

@Vlatombe

lgtm

@jglick jglick merged commit d1eaafb into jenkinsci:master May 3, 2018

2 checks passed

continuous-integration/jenkins/incrementals Deployed to Incrementals.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@jglick jglick deleted the jglick:InstallState-madness-redux branch May 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment