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-62757] Introduce fingerprint migration in External Storage API #4825

Merged
merged 18 commits into from
Aug 1, 2020

Conversation

stellargo
Copy link
Contributor

@stellargo stellargo commented Jun 29, 2020

See JENKINS-62757.
See JEP-226.

Proposed changelog entries

  • Developer: Migration of fingerprints from local file based storage to configured external storage is introduced.

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@stellargo
Copy link
Contributor Author

This PR is WIP currently.

@res0nance res0nance added the work-in-progress The PR is under active development, not ready to the final review label Jun 29, 2020
@stellargo stellargo changed the title [JENKINS-62757] Introduce fingerprint migration [JENKINS-62757] Introduce fingerprint migration in External Storage API Jul 1, 2020
@stellargo
Copy link
Contributor Author

@oleg-nenashev @afalko @mikecirioli This is the migration PR. Work is done but suggest putting it on hold till #4834 is merged.

Copy link

@afalko afalko left a comment

Choose a reason for hiding this comment

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

Small question and comment improvement suggestion

@stellargo stellargo marked this pull request as ready for review July 21, 2020 01:41
@stellargo stellargo requested a review from afalko July 21, 2020 04:51
@stellargo
Copy link
Contributor Author

@afalko @oleg-nenashev @mikecirioli request for review :)

@oleg-nenashev oleg-nenashev added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted and removed work-in-progress The PR is under active development, not ready to the final review labels Jul 23, 2020
@oleg-nenashev oleg-nenashev requested review from a team and oleg-nenashev July 23, 2020 14:45
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 would recommend to use local variables to make the code more reliable against concurrency issues

core/src/main/java/hudson/model/Fingerprint.java Outdated Show resolved Hide resolved
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.

Should be good to go, thanks @stellargo !

We may merge it in 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jul 27, 2020
@stellargo stellargo marked this pull request as draft July 27, 2020 13:39
@stellargo stellargo marked this pull request as ready for review July 27, 2020 13:46
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.

👍 for catching the bug @stellargo . Resetting the merge timer

We may merge it in 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process

@timja timja merged commit 885c05c into jenkinsci:master Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants