Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add parameter to set user uuid in jenkins::credentials define #227

Merged
merged 5 commits into from
Aug 9, 2015

Conversation

madAndroid
Copy link
Contributor

This addresses #226 - It's largely influenced by the Opscode jenkins module, which has this functionality - https://github.com/opscode-cookbooks/jenkins/blob/master/libraries/credentials.rb

@jenkinsadmin
Copy link

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@rtyler
Copy link

rtyler commented Dec 30, 2014

Ooooo, with tests! I'll try to review this some time this week and try it out!

@madAndroid madAndroid force-pushed the allow_credentials_uuid branch from 4a6c226 to dacaa68 Compare February 6, 2015 05:56
@madAndroid
Copy link
Contributor Author

@rtyler - Any progress on testing this? :) Please let me know if more tests are required

@xiii
Copy link

xiii commented May 24, 2015

@rtyler @madAndroid that would be very useful! hope it gets merged soon!

@jhoblitt
Copy link
Member

The last CI run failed for this PR and it needs to be rebased on the current master due to changes introduced by #285.

@madAndroid
Copy link
Contributor Author

Cool, no worries - I'll rebase and sort out any conflicts

@madAndroid madAndroid force-pushed the allow_credentials_uuid branch from dacaa68 to ba47318 Compare May 24, 2015 18:24
@madAndroid
Copy link
Contributor Author

Rebased, and tests fixed :) ( https://travis-ci.org/jenkinsci/puppet-jenkins/builds/63873665 )

@@ -170,10 +170,13 @@ class Actions {
)[0].getStore()

def credentials
if (id == "") {
id = null
}
Copy link
Member

Choose a reason for hiding this comment

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

From looking at the existing code, I thought that the cli argument to method call conversion, actions."$action"(*args[1..-1]), must be requiring string params to be defaulted to an empty string. However, I just tested it and there doesn't appear to be an issue with defaulting a string param to null.

The id param could be defaulted to null and then this check wouldn't be needed.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, that might not make any sense. I'm not sure how you would pass null from the cli.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how you would pass null from the cli.

That's pretty much the issue I came up against, which is why I ended up taking this route.

@jhoblitt
Copy link
Member

👍 This looks good to me!

CI didn't run; travis has been acting strange the last few days. I've had to force push some of my own branches in order to get it trigger.

@madAndroid
Copy link
Contributor Author

CI didn't run; travis has been acting strange the last few days. I've had to force push some of my own branches in order to get it trigger.

Odd, I did see the run on travis after I pushed the latest changes up .. results are here, AFAICT -> https://travis-ci.org/jenkinsci/puppet-jenkins/builds/63873665

@madAndroid
Copy link
Contributor Author

An example of the use case where you may want to set the UUID during credential creation: JJB's SCM plugin allows you to set credential-id field, which allows you to have a predefined UUID for easy rebuild of envs

@madAndroid madAndroid force-pushed the allow_credentials_uuid branch from ea444bc to d473125 Compare June 23, 2015 14:52
@madAndroid
Copy link
Contributor Author

Is there anything more I can do to have this request merged? ( @jhoblitt / @rtyler )

@jhoblitt
Copy link
Member

This looks pretty good to me. Mentioning the new param in the README might not hurt. (eg., https://github.com/jenkinsci/puppet-jenkins#credentials)

One thing of note is that this does change the order of params to create_or_update_credential -- I'm not sure if that's ever been considered a public API, @rtyler ?

@madAndroid madAndroid force-pushed the allow_credentials_uuid branch from d473125 to afb3a1a Compare July 28, 2015 05:17
@madAndroid
Copy link
Contributor Author

@rtyler - anything more I need to do to have this merged? I've added tests, would you like me to expand upon those?

@madAndroid madAndroid force-pushed the allow_credentials_uuid branch from ace89a3 to abab419 Compare August 9, 2015 15:03
@madAndroid madAndroid force-pushed the allow_credentials_uuid branch from abab419 to cff1113 Compare August 9, 2015 15:06
@madAndroid
Copy link
Contributor Author

Hi, is there anything further than can be done to have this merged? Do you need me to justify the reason for the addition, and perhaps explain and describe our use case?

@jhoblitt - in response to your query: the ordering of args in create_or_update_credential is taken directly from the credentials-plugin source: https://github.com/jenkinsci/credentials-plugin/blob/master/src/main/java/com/cloudbees/plugins/credentials/impl/UsernamePasswordCredentialsImpl.java#L56-L64
ie:

    /**
     * Constructor.
     *
     * @param scope       the credentials scope
     * @param id          the ID or {@code null} to generate a new one.
     * @param description the description.
     * @param username    the username.
     * @param password    the password.
     */

I've rebased against the latest HEAD of master, and fixed tests that changed as a result of another merge.

@rtyler rtyler added this to the 1.5.0 - Jennings milestone Aug 9, 2015
@rtyler
Copy link

rtyler commented Aug 9, 2015

@madAndroid nope, thanks for the bump and the good contribution :)

rtyler pushed a commit that referenced this pull request Aug 9, 2015
Add parameter to set user uuid in jenkins::credentials define
@rtyler rtyler merged commit 040b271 into voxpupuli:master Aug 9, 2015
@madAndroid
Copy link
Contributor Author

Awesome, thanks! :-)
On 9 Aug 2015 5:58 pm, "R. Tyler Croy" notifications@github.com wrote:

@madAndroid https://github.com/madAndroid nope, thanks for the bump and
the good contribution :)


Reply to this email directly or view it on GitHub
#227 (comment)
.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants