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-38048] Permit credentialsId dropdowns to be used from a global configuration screen #437

Merged
merged 4 commits into from Oct 29, 2016

Conversation

Projects
None yet
4 participants
@jglick
Copy link
Member

jglick commented Sep 8, 2016

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Sep 8, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Sep 8, 2016

Complements #433.

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Sep 9, 2016

🐝

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Sep 9, 2016

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Sep 9, 2016

Thanks. I'm deep in items at work today, then plan to spend much of the weekend preparing for the release of git plugin 3.0 and git client plugin 2.0 (add submodule authentication, switch to JDK 7, depend on Jenkins 1.625 or newer, etc.). Can this wait until after Jenkins World, or do you need this (and #433) prior to Jenkins World next week?

I may be able to evaluate this during the hackfest late next week, if others don't mind me focusing on the git plugin and git client plugin during the hackfest.

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Sep 9, 2016

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Sep 9, 2016

do you need this (and #433) prior to Jenkins World

Well, if you happened to release #433 very soon, I might be able to incorporate it into demos of external Pipeline libraries, but I was certainly not relying on it. There is a fallback implementation (Legacy SCM) which works with existing Git plugin releases, just not quite as nicely.

See you soon!

@@ -185,7 +188,7 @@ public FormValidation doCheckUrl(@AncestorInPath Item item,
return FormValidation.ok();
}

private static StandardCredentials lookupCredentials(Item project, String credentialId, String uri) {
private static StandardCredentials lookupCredentials(@CheckForNull Item project, String credentialId, String uri) {
return (credentialId == null) ? null : CredentialsMatchers.firstOrNull(
CredentialsProvider.lookupCredentials(StandardCredentials.class, project, ACL.SYSTEM,

This comment has been minimized.

@stephenc

stephenc Sep 20, 2016

Member

I'd rather be explicit in plugins about what to fall back to if the project is null... perhaps we should discuss this and jenkinsci/credentials-plugin#68

This comment has been minimized.

@jglick

jglick Sep 21, 2016

Author Member

lookupCredentials already accepted null. Perhaps you meant to comment on a different line?

jglick added a commit to jenkinsci/workflow-cps-global-lib-plugin that referenced this pull request Oct 3, 2016

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Oct 24, 2016

@MarkEWaite claims this patch is not working for him, so I will merge against current master and try to add test coverage.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Oct 24, 2016

BTW the branching strategy in this plugin has left me a bit confused, since master is creating 2.6.1-SNAPSHOT; but there is a 3.0.0 (nonbeta) release and a 3.0.0-beta branch. Assuming for now that this is all irrelevant for purposes of this PR.

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Oct 24, 2016

You're very polite to call the current branching of the plugin confusing. My apologies. 3.0.0-beta branch is (for the moment) the most recently released branch on the stream.

Once PR433 and PR437 have been merged to the master and to 3.0.0-beta, I intend to change the branching so that the master branch will match with the 3.0.0-beta branch and will again be the branch used for future releases of the plugin

The 2.6.x branch will be the branch from which any releases of the 2.6 version of the plugin are released.

Sorry for the confusion created by the my not making the branch transition on the git plugin that I've already made on the git client plugin.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Oct 24, 2016

No problem. Sounds like it is fine for this to continue to be based on master.

I cannot reproduce any problem after merging with master, updating credentials (to 2.1.7), and running with workflow-cps-global-lib. The credentials I have defined:

creds

are available as expected:

config

Nonetheless I will do a bit more interactive testing and add a functional test.

@@ -85,7 +85,8 @@ public String toString() {
public ListBoxModel doFillCredentialsIdItems(@AncestorInPath Item project,
@QueryParameter String url,
@QueryParameter String credentialsId) {
if (project == null || !project.hasPermission(Item.EXTENDED_READ)) {
if (project == null && !Jenkins.getActiveInstance().hasPermission(Jenkins.ADMINISTER) ||
project != null && !project.hasPermission(Item.EXTENDED_READ)) {

This comment has been minimized.

@jglick

jglick Oct 24, 2016

Author Member

This is the diff hunk currently covered by the functional test—the one most important for the reported bug.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Oct 29, 2016

Looks like I need to resolve some merge conflict here; will try to do it soon.

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Oct 29, 2016

I think I've already resolved them in the version I'm testing. It is a simple change to UserRemoteConfig.java

All my interactive test and test automation updates based on this are looking good. I've switched many of the Jenkinsfile references in jenkins-bugs to use a global pipeline. I've been through tests which use a different forms of global pipeline references and have learned a bunch.

@MarkEWaite MarkEWaite merged commit 1c20b76 into jenkinsci:master Oct 29, 2016

1 check failed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Oct 29, 2016

Merged.

@jglick jglick deleted the jglick:global-credentialsId-JENKINS-38048 branch Oct 31, 2016

@@ -82,10 +83,12 @@ public String toString() {
@Extension
public static class DescriptorImpl extends Descriptor<UserRemoteConfig> {

@SuppressFBWarnings(value="NP_NULL_PARAM_DEREF", justification="pending https://github.com/jenkinsci/credentials-plugin/pull/68")

This comment has been minimized.

@jglick

jglick Oct 31, 2016

Author Member

Note that this was merged so this comment could be amended to say that the suppression could be deleted as of credentials 2.1.9.

@@ -189,10 +190,12 @@ public String getDisplayName() {
return Messages.GitSCMSource_DisplayName();
}

@SuppressFBWarnings(value="NP_NULL_PARAM_DEREF", justification="pending https://github.com/jenkinsci/credentials-plugin/pull/68")

This comment has been minimized.

@jglick

jglick Oct 31, 2016

Author Member

Ditto here of course.

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.