-
-
Notifications
You must be signed in to change notification settings - Fork 9k
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-61046] - Allow plugins to force an update of the UpdateCenter #4488
[JENKINS-61046] - Allow plugins to force an update of the UpdateCenter #4488
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me in principle, but IMO we should avoid exposing test parameters in new API. I suggest to introduce a new API method which does not allow disabling the signature check. (may be off only for testing!)
in Javadoc was a good first step, but IMHO it is not enough
if we're introducing a new API then it would be good for it not to return form validation and either return a boolean or throw on error |
@oleg-nenashev notes that the public API should not expose the security option that is for testing. Introduced updateDirectlyNow() as the new API and updateDirectly() to have a mirror and deprecated updateDirectly(boolean) as a consequence.
@timja that would collide with the way I have just addressed @oleg-nenashev's feedback. internal non test code no longer call before I make any further changes could you both come to an agreement on the latest or disagree on the merits of 20a2c76 |
fixes the grammar of the UpdateSite#updateDirectlyNow javadoc Co-Authored-By: Raihaan Shouhell <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if an intended API didn’t return web specific form validation, but not blocking it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jtnord ! The current version looks good to me.
*/ | ||
@Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bonus points for restricting the external use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would break source compatibility, and that could break some (at least internal) workflows, as well as having the potential to force disabling the Restrictions
check a for another year - which was the whole point of this PR to begin with.
Let's just at least deprecate it for at least a few LTS version first (we could/should start looking at doing a sweep of all deprecated API and look at restrict them based on how long they have been deprecated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me. We break compatibility too often in recent releases
I suggest merging it tomorrow if no negative feedback |
The 2.221 release did not include the following items which were incorrectly listed in the original 2.221 changelog: * New permission Overall/Manage - JEP-223, [PR-4501](jenkinsci/jenkins#4501) * Memory usage monitor in system info page - [PR-4499](jenkinsci/jenkins#4499) * Allow plugins to force an update of UpdateCenter - [PR-4488](jenkinsci/jenkins#4488) * Admin monitors sorted in global config page - [PR-4487](jenkinsci/jenkins#4487) * Tied job loading performance improvement - [PR-4497](jenkinsci/jenkins#4497)
See JENKINS-61046.
Proposed changelog entries
UpdateSite
Proposed upgrade guidelines
N/A
Submitter checklist
Proposed changelog entries
section only if there are breaking changes or other changes which may require extra steps from users during the upgradeDesired reviewers
@mention
Maintainer checklist
Before the changes are marked as
ready-for-merge
:Proposed changelog entries
are correctupgrade-guide-needed
label is set and there is aProposed upgrade guidelines
section in the PR title. (example)lts-candidate