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

Update processcreds.CredentialProcessResponse visibility to public #1921

Merged
merged 1 commit into from
Feb 3, 2023

Conversation

joshdk
Copy link
Contributor

@joshdk joshdk commented Nov 12, 2022

In order to have a canonical type for encoding/decoding JSON for a credential process, the visibility of this type needed to be public. This type has been detailed in official AWS CLI documentation and is effectively part of the tooling ecosystem's external interface.

@joshdk joshdk requested a review from a team as a code owner November 12, 2022 05:01
Copy link
Contributor Author

@joshdk joshdk left a comment

Choose a reason for hiding this comment

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

For context - I am authoring a custom tool for use as a credential_process, which requires a type that I can instantiate and json.Marshal to stdout.

I am currently duplicating this type, but referencing a canonical type from the package (which is also handling AWS profile manipulation) would be the semantically correct thing, and would avoid inadvertent tool breakages when e.g. upgrading the SDK version.

For consistency, and identical change was made in aws/aws-sdk-go#4621.

Thank you for your consideration!

// A CredentialProcessResponse is the AWS credentials format that must be
// returned when executing an external credential_process.
type CredentialProcessResponse struct {
// As of this writing, the Version key must be set to 1. This might
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment taken from AWS CLI credential_process authoring documentation.

See https://docs.aws.amazon.com/sdkref/latest/guide/feature-process-credentials.html#feature-process-credentials-output.

// increment over time as the structure evolves.
Version int

// The access key ID that identifies the temporary security credentials.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments copied from the types.Credentials struct fields.

See https://github.com/aws/aws-sdk-go-v2/blob/main/service/sts/types/types.go#L34.

@joshdk
Copy link
Contributor Author

joshdk commented Nov 19, 2022

Friendly 1-week ping if anyone would like to approve the CI workflows 🙂

@joshdk
Copy link
Contributor Author

joshdk commented Dec 12, 2022

Friendly 1-month ping for some engagement 🙂

@aajtodd
Copy link
Contributor

aajtodd commented Feb 1, 2023

@joshdk

Thanks for the PR and sorry for the delay. I think I agree that this is effectively part of the overall ecosystem tooling interface. Making this type public should be ok. It looks like CI checks didn't run yet, can you try and push up an empty commit to see if it triggers an approval? Thanks.

In order to have a canonical type for encoding/decoding JSON for a credential
process, the visibility of this type needed to be public. This type has been
detailed in official AWS CLI documentation and is effectively part of the
tooling ecosystem's external interface.
@joshdk
Copy link
Contributor Author

joshdk commented Feb 2, 2023

Thank you so much @aajtodd!

I rebased off of the latest main and it appears that the CI checks are now waiting for a maintainer to approve.

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