-
Notifications
You must be signed in to change notification settings - Fork 66
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-73408] bom update to bom-2.426.x in order to align with core requirement #211
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.
thanks, I think this may fail in some cases though
@@ -92,8 +92,8 @@ | |||
<dependencies> | |||
<dependency> | |||
<groupId>io.jenkins.tools.bom</groupId> | |||
<artifactId>bom-2.387.x</artifactId> | |||
<version>2543.vfb_1a_5fb_9496d</version> | |||
<artifactId>bom-2.426.x</artifactId> |
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 worth extracting to a variable so the next person doesn't make the same mistake
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.
If you want to offer such advice, you should do it first in archetypes
and demonstrate that it is comprehensible to @dependabot etc.
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.
@@ -26,6 +26,6 @@ | |||
<?jelly escape-by-default='true'?> | |||
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form"> | |||
<f:entry title="${%Key}" field="privateKey"> | |||
<f:secretTextarea checkMethod="post" id="${keyId}"/> | |||
<f:secretTextarea checkMethod="post" id="sshCredentials_privateKey"/> |
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.
what if multiple of these are on the same page?
generally you want a class name not an ID for targeting JavaScript on
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 for your review :-)
Should not happen as this is only accepting one entry, but better safe than sorry.
Changed it to use class name (had to use clazz
in one field and class
in the other) and using only the first occurrence (if any)
fd99628
...s/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey/credentials.jelly
Outdated
Show resolved
Hide resolved
...loudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey/passphraseChangeEvent.js
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,13 @@ | |||
//TODO: this snippet, as well as ids in passphrase and private key fields can be removed once https://issues.jenkins.io/browse/JENKINS-65616 is completed |
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.
Unless you (attempt to) implement that, it is not likely anyone ever will.
All these hacks could go away by just deprecating DirectEntryPrivateKeySource
(we do not support any other source anyway, for security reasons) and just make the privateKey
a field of BasicSSHUserPrivateKey
directly.
Hi, FYI this version (341.vf31377f30378) completely breaks the plugin loading on startup as well as crashes Jenkins itself. I'm on LTS 2.452.3, and currently restricting this plugin version to 337.v395d2403ccd4 to get it to start again. |
@IppX you are going to need to be more specific. A stack trace, to start with? |
Sure, here is what I get. You have to read it bottom up. startup log
|
I think that message indicates that you need a newer version of the trilead API plugin in your configuration. The most recent release of the ssh-credentials loads correctly in my plugins.txt file |
The plugin bill of materials tests fail with a different message on this version compared to the previous version. More details in the revert pull request at: The issue seems to be the incorrect handling of optional dependencies by the plugin compatibility tester. I hope to spend some time trying to find a workaround today. |
The plugin compatibility tester mistakenly uses the version of optional plugins that is provided by the pom of the tested plugin rather than using the version of plugin that is listed in the pom file of the plugin bill of materials. Because of that issue, the 2.426.x plugin bill of materials version needs to remain on an older version in order to not include an optional dependency on a newer version of the trilead API plugin. 3080.vfa_b_e4a_a_39b_44 is the version immediately before a trilean API plugin upgrade in the BOM. jenkinsci/bom#3404 has more details jenkinsci#211 includes some discussion as well.
The plugin compatibility tester mistakenly uses the version of optional plugins that is provided by the pom of the tested plugin rather than using the version of plugin that is listed in the pom file of the plugin bill of materials. Because of that issue, the 2.426.x plugin bill of materials version needs to remain on an older version in order to not include an optional dependency on a newer version of the trilead API plugin. 3080.vfa_b_e4a_a_39b_44 is the version immediately before a trilean API plugin upgrade in the BOM. jenkinsci/bom#3404 has more details #211 includes some discussion as well.
Thanks for sharing this, looks like I can learn a few things from how you build your Jenkins instance ;) |
Bumping BOM after change done in #210
Raised by jenkinsci/bom#3399
Addresses #210 (comment) and #210 (comment)
Additionally, excluded eddsa-api (won't allow starting up in FIPS mode) and updated tests to exclude it
Also, recovered adjunct for javascript snippet instead of inline scripting
Testing done
Tests updated, no change in the logic itself
Submitter checklist