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-46247] Fix credentials scope in withAWS step and add a crede… #16

Merged
merged 4 commits into from Sep 7, 2017

Conversation

Projects
None yet
4 participants
@Dohbedoh
Copy link
Contributor

commented Aug 31, 2017

…ntials dropdown

@reviewbybees

@reviewbybees

This comment has been minimized.

Copy link

commented Aug 31, 2017

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.

@Dohbedoh

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2017

Tested the Descriptor locally:

listboxmodel

@hoegertn

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2017

Looks good. I will test it.

There are some commented lines. Do you want to remove them before merging?
Should I wait for another reviewer?

@Dohbedoh

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2017

Thanks. Let me remove the comments.

Should I wait for another reviewer?

With Jenkins World on, not sure if we will get a review this week. The build is failing so it may be best ? The tests pass locally.

@hoegertn

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2017

The failing build seems to be a problem with TravisCI. Maybe I will try other build engines or disable them completely.

@Dohbedoh

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2017

Got it.
I cleaned up the comments. I also noticed I was using methods from previous versions of the credentials API. I switched to the recommended one.

@Dohbedoh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2017

@hoegertn Note that you can simply add a Jenkinsfile with buildPlugin()and use https://ci.jenkins.io/

@hoegertn

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2017

Sounds great. Will give it a try.

@hoegertn

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2017

Please rebase onto master, as the build is fixed.

@Dohbedoh Dohbedoh force-pushed the Dohbedoh:JENKINS-46247 branch from 9e49fed to 2184b41 Sep 4, 2017

@cyrille-leclerc
Copy link

left a comment

Very nice. Some suggestions added but nothing requiring changes.

CredentialsMatcher matcher = CredentialsMatchers.withId(this.step.getCredentials());
UsernamePasswordCredentials usernamePasswordCredentials = CredentialsMatchers.firstOrNull(credentials, matcher);
StandardUsernamePasswordCredentials usernamePasswordCredentials = CredentialsProvider.findCredentialById(this.step.getCredentials(),
StandardUsernamePasswordCredentials.class, run, Collections.<DomainRequirement>emptyList());
if (usernamePasswordCredentials != null) {

This comment has been minimized.

Copy link
@cyrille-leclerc

cyrille-leclerc Sep 6, 2017

Note: I like to add a warning message if the desired credentials have NOT been found. With the portability of Jenkinsfiles and copy/paste, it's quite easy to have an invalid credentials ID and it would be hard to understand

This comment has been minimized.

Copy link
@Dohbedoh

Dohbedoh Sep 7, 2017

Author Contributor

Isn't the RuntimeException enough here ? Do you mean a warning in the console output ?

This comment has been minimized.

Copy link
@cyrille-leclerc

cyrille-leclerc Sep 7, 2017

my mistake,, I didn't see the "else" block, I didn't expect a "double negation". The Runtime Exception is indeed a clear message.

? Tasks.getAuthenticationOf((Queue.Task) context)
: ACL.SYSTEM,
context,
StandardUsernamePasswordCredentials.class,

This comment has been minimized.

Copy link
@cyrille-leclerc

cyrille-leclerc Sep 6, 2017

Note: this plugin prefers to use the StandardUsernamePasswordCredentials instead of the AmazonWebServicesCredentials. There are pros and cons.
Support both types would be nice but I'm not sure it is easy.

This comment has been minimized.

Copy link
@Dohbedoh

Dohbedoh Sep 7, 2017

Author Contributor

The only cons I can think of is that AmazonWebServicesCredentials carries a redundant information (the role). The withAWS step has a parameter for this already. So it could be confusing or it could be integrated accordingly with one taking precedence (the role specified in the withAWS step) over the other (the one specified in the AWS credentials).

Support both types would be nice but I'm not sure it is easy.

As far as I can tell it would require to find credentials of type StandardCredentials and then do a conditional on the type of implementation. Could be part of a different issue maybe ?

This comment has been minimized.

Copy link
@cyrille-leclerc

cyrille-leclerc Sep 7, 2017

Could be part of a different issue maybe ?

It would definitively be a different Jira issue. I'm not sure that the Credentials API easily supports it at the moment.

@hoegertn hoegertn merged commit 044f460 into jenkinsci:master Sep 7, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
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.