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

IdStore.getId() and make() methods support the non-migrated data as a fallback #1

Closed
wants to merge 2 commits into from

Conversation

oleg-nenashev
Copy link

This change allows to operate properly if the data has not been migrated correctly or has been loaded after the start (Job Config History, Manual copy, etc.). Also extends and fixes unit tests.

@reviewbybees

… fallback.

This change allows to operate properly if the data has not been migrated correctly or has been loaded after the start (Job Config History, Manual copy, etc.). Also extends and fixes unit tests.
@oleg-nenashev
Copy link
Author

FAQ: Yes, I want to merge it to @jtnord 's branch

@jtnord
Copy link
Owner

jtnord commented Jun 11, 2015

@oleg-nenashev There should not be a need for the legacy lookup.
If the legacy migration fails then this will bomb out and it should be re-attempted at next startup.

The legacy actions must be removed - otherwise the root issue still exists (an item when copied may return duplicate information).

@oleg-nenashev
Copy link
Author

@jtnord
We cannot guarantee that the user does not load the config after the restart somehow.
What about also patching JobIdProperty.onLoad()?

@jtnord
Copy link
Owner

jtnord commented Jun 11, 2015

@oleg-nenashev if they load the config after restart - then it is likely that the user has copied the job - in which case we will be migrating bad data.
We will already have to document that if the user has copied jobs they will need to do something to fix things (as we can't just arbitrarily create a new unique string as that will break everything that used the old unique string if there are no conflicts.) Conflicts can exist on different masters so we can not even hope to keep track of these (allthough we could make a best effort to do so and spit out a warning...)

@oleg-nenashev
Copy link
Author

I don't completely agree with such approach if we use a multi-instance Jenkins installation, which transfer jobs between each other somehow. Without this patch there won't be an opportunity of one-by-one upgrade.

@jtnord jtnord closed this Jun 11, 2015
jtnord added a commit that referenced this pull request Jun 11, 2015
[FIXED JENKINS-28843] No longer stored the unique value in the items configuration.
jtnord added a commit that referenced this pull request Jun 11, 2015
Tweak logging levels andload actions to force lazy loading.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants