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

Permit BasicSSHUserPrivateKey.id to be configured #12

Merged
merged 2 commits into from Jan 15, 2015

Conversation

Projects
None yet
2 participants
@jglick
Copy link
Member

commented Jan 14, 2015

</f:invisibleEntry>
<j:if test="${instance==null}">
<j:set var="instance" value="${descriptor.fixInstance(instance)}"/>
</j:if>

This comment has been minimized.

Copy link
@jglick

jglick Jan 14, 2015

Author Member

As in jenkinsci/credentials-plugin#21 I could not find the purpose for this code, and deleted it since it was blocking the change here. I tried creating and updating SSH credentials using all three key sources and it seemed fine. In the absence of any automated tests verifying the supported web interactions, I am not sure what else to check.

<f:entry title="${%Username}" field="username">
<f:textbox/>
</f:entry>
<f:entry title="${%Description}" field="description">
<f:textbox/>
</f:entry>
<f:entry title="${%Private Key}" field="privateKeySource">
<!-- TODO switch back to hetero-radio when the initial expansion bug is fixed -->

This comment has been minimized.

Copy link
@jglick

jglick Jan 14, 2015

Author Member

So what was that bug? The commit comment just mentions working around a “Jenkins UI glitch” but gives no bug number or indication of when or whether it might be fixed in core.

<j:set var="currentInstance" value="${instance.privateKeySource}" />
<j:set var="currentDescriptor" value="${currentInstance.descriptor}" />
<j:set var="currentInstance" value="${instance != null ? instance.privateKeySource : null}"/>
<j:set var="currentDescriptor" value="${currentInstance != null ? currentInstance.descriptor : null}"/>

This comment has been minimized.

Copy link
@jglick

jglick Jan 14, 2015

Author Member

Note that this means that no radio button is selected initially. Since the privateKeySource is mandatory and prominent, I do not see a problem with that.

</f:advanced>
<f:entry title="${%Passphrase}" field="passphrase">
<f:password/>
</f:entry>

This comment has been minimized.

Copy link
@jglick

jglick Jan 14, 2015

Author Member

Note that I am moving passphrase out of its Advanced block, since it would be awkward to have two. Anyway this does not seem so advanced to me; many people advocate encrypting your private keys (though of course the benefit of doing so is reduced when files in $JENKINS_HOME can be used to decrypt them).

pom.xml Outdated
@@ -96,7 +96,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>credentials</artifactId>
<version>1.16.1</version>
<version>1.21-20150114.010642-1</version>

This comment has been minimized.

Copy link
@jglick

jglick Jan 14, 2015

Author Member

Pending the upstream merge and release, of course.

This comment has been minimized.

Copy link
@stephenc

stephenc Jan 15, 2015

Member

Well we are WIP until you get the upstream done then!

This comment has been minimized.

Copy link
@jglick

jglick Jan 15, 2015

Author Member

Obviously, but other than that?

@stephenc

This comment has been minimized.

Copy link
Member

commented Jan 15, 2015

+1 once upstream is released and the diff is updated to reflect the release version

@jglick

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2015

Diff updated. Still awaiting a second @reviewbybees. Could perform a plugin release after this if desired, or @stephenc could, or it could wait.

@stephenc

This comment has been minimized.

Copy link
Member

commented Jan 15, 2015

Meh! you're 12h+

jglick added a commit that referenced this pull request Jan 15, 2015

Merge pull request #12 from jenkinsci/config-id-JENKINS-26099
Permit BasicSSHUserPrivateKey.id to be configured

@jglick jglick merged commit eee394f into master Jan 15, 2015

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the config-id-JENKINS-26099 branch Jan 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.