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-72998] call refresh on the Extension providers again #9157

Merged
merged 1 commit into from
Apr 14, 2024

Conversation

jtnord
Copy link
Member

@jtnord jtnord commented Apr 11, 2024

In the case the ExtensionFinder is itself extensible we refresh it again to catch extensions that would be discovered by a newly installed extension.

This seems a little overzerlous, as there is only the variant plugin that I know of, however calling Jenkins.get().refreshExtensions() from any point in the variant plugin would seem to be a little bit dangerous as it would call back into a place where the reactor is already calling back from.

See JENKINS-72998.

Testing done

started a clean jenkins without any plugins
installed ssh-slaves, ssh-credentails & credentials
Attempted to create a new SSH based slave with a credential

validated credentials are not listed before this change and they are after
see jenkinsci/acceptance-test-harness#1526

incremental build tested with ATH

Proposed changelog entries

  • JENKINS-72998, If the variant plugin is installed at the same time as a plugin that has an OptionalExtension then these extensions would not be correctly discovered, until the next scan for new Extensions.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

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

Maintainer checklist

In the case the ExtensionFinder is itself extensible we refresh it again
to catch extensions that would be discovered by a newly installed
extension.

This seems a little overzerlous, as there is only the variant plugin
that I know of, however calling `Jenkins.get().refreshExtensions() from
any point in the `variant` plugin would seem to be a little bit
dangerous as it would call back into a place where the reactor is
already calling back from.
@jtnord jtnord requested review from jglick and Vlatombe April 11, 2024 15:59
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

🙈

Is it possible to mark the variant plugin as not supporting hot reload, as a less risky change?

@jtnord
Copy link
Member Author

jtnord commented Apr 11, 2024

🙈

Is it possible to mark the variant plugin as not supporting hot reload, as a less risky change?

It's possible (it's just code!) but it is a really bad ux as [ssh-slaves] (

{ "name": "ssh-slaves", "suggested": true }
) is a suggested plugin in platform-plugins and this would mean the default install flow would now require a restart, due to its dependency on ssh-credentials which optionally depends on trilead-api via OptionalExtension and ssh-shaves has a direct dependency on trilead-api

@jtnord jtnord changed the title [JENKINS-58302] call refresh on the Extension providers again [JENKINS-72998] call refresh on the Extension providers again Apr 12, 2024
@jtnord
Copy link
Member Author

jtnord commented Apr 12, 2024

https://ci.jenkins.io/job/Core/job/acceptance-test-harness/job/PR-1528/2/testReport/ shows 2 consistend new failures with this PR.. investigating.

@jtnord
Copy link
Member Author

jtnord commented Apr 12, 2024

https://ci.jenkins.io/job/Core/job/acceptance-test-harness/job/PR-1528/2/testReport/ shows 2 consistend new failures with this PR.. investigating.

fails when run with latest master also so is unrelated to this change

@jtnord jtnord marked this pull request as ready for review April 12, 2024 15:44
@timja timja added the bug For changelog: Minor bug. Will be listed after features label Apr 12, 2024
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 12, 2024
@NotMyFault NotMyFault merged commit cb89cad into jenkinsci:master Apr 14, 2024
16 checks passed
NotMyFault pushed a commit to NotMyFault/jenkins that referenced this pull request Apr 28, 2024
…sci#9157)

[JENKINS-58302] call refresh on the Extension providers again

In the case the ExtensionFinder is itself extensible we refresh it again
to catch extensions that would be discovered by a newly installed
extension.

This seems a little overzerlous, as there is only the variant plugin
that I know of, however calling `Jenkins.get().refreshExtensions() from
any point in the `variant` plugin would seem to be a little bit
dangerous as it would call back into a place where the reactor is
already calling back from.

(cherry picked from commit cb89cad)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Minor bug. Will be listed after features ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants