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

feat: add External ID parameter to AWS converter #57

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

artydeez
Copy link

@artydeez artydeez commented Jan 25, 2022

certain credential plugins, like the credential-binding plugin, require that the externalID be specified when using IAM Roles and the aws credential type.

This will add the ability to optionally add iamExternalId in the same way that iamRoleArn is added

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution Ryan.

This looks great, but would you be willing to tweak the doc example and add an extra assert to the unit test?

assertThat("credential scope is mapped correctly", credential.getScope(), is(CredentialsScope.GLOBAL));
assertThat("credential accessKey is mapped correctly", credential.getAccessKey(), is(accessKey));
assertThat("credential secretKey is mapped correctly", credential.getSecretKey().getPlainText(), is(secretKey));
assertThat("credential iamRoleArn is mapped correctly", credential.getIamRoleArn(), is(iamRoleArn));
Copy link
Member

Choose a reason for hiding this comment

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

Should you add a check that the external id check
is null here?

Copy link
Author

Choose a reason for hiding this comment

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

@jtnord can you elaborate or add an example? None of the other missing item validations are explicitely checking if that value is null

Copy link
Member

Choose a reason for hiding this comment

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

@@ -0,0 +1,17 @@
apiVersion: v1
Copy link
Member

Choose a reason for hiding this comment

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

This isn't referenced from the examples page (or elsewhere?
Could you update the existing Aws example with some sommebts for optionality of the fields?

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand. Do you mean that I should add a reference within the examples README?

Copy link
Member

Choose a reason for hiding this comment

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

Yes these docs/ will not be visible I documentation unless you reference them somewhere.
https://github.com/jenkinsci/kubernetes-credentials-provider-plugin/blob/master/docs/examples/README.md#aws-credentials

@jtnord
Copy link
Member

jtnord commented Jul 14, 2022

@rdonahoe-placed do you plan to address the build failure (compilation issue)?

[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /home/jenkins/workspace/redentials-provider-plugin_PR-57/src/main/java/com/cloudbees/jenkins/plugins/kubernetes_credentials_provider/convertors/AWSCredentialsConvertor.java:[79,15] error: no suitable constructor found for AWSCredentialsImpl(CredentialsScope,String,String,String,String,String,String,String)
     constructor AWSCredentialsImpl.AWSCredentialsImpl(CredentialsScope,String,String,String,String) is not applicable
       (actual and formal argument lists differ in length)
     constructor AWSCredentialsImpl.AWSCredentialsImpl(CredentialsScope,String,String,String,String,String,String) is not applicable
       (actual and formal argument lists differ in length)

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

Successfully merging this pull request may close these issues.

2 participants