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 ecr login #36

Merged
merged 25 commits into from
Dec 19, 2022
Merged

feat: Add ecr login #36

merged 25 commits into from
Dec 19, 2022

Conversation

victorgu-github
Copy link
Collaborator

What does this PR do?

get ecr token to pull images/charts from public ECR

🛑 Please open an issue first to discuss any significant work and flesh out details/direction - we would hate for your time to be wasted.
Consult the CONTRIBUTING guide for submitting pull-requests.

Motivation

  • Resolves #

Test Results

Additional Notes

@victorgu-github victorgu-github requested a review from a team as a code owner December 14, 2022 19:04
@victorgu-github
Copy link
Collaborator Author

@bryantbiggs i tested locally. it works.
it needs data "aws_ecr_authorization_token" "ecr_token" {}
and local-exec

@askulkarni2 askulkarni2 changed the title [enhance] add ecr login feat: Add ecr login Dec 19, 2022
Copy link
Contributor

@bryantbiggs bryantbiggs left a comment

Choose a reason for hiding this comment

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

looks great!

@victorgu-github victorgu-github merged commit b9e31e3 into aws-ia:main Dec 19, 2022
@vara-bonthu
Copy link

Should the ecr login use us-east-1 region?

@bryantbiggs
Copy link
Contributor

thats a good point - I think we will have to show users how to pass this in, and make that as easy as possible (expose as top level variables) so that we don't embed the provider into the module

@bryantbiggs
Copy link
Contributor

bryantbiggs commented Dec 19, 2022

so to clarify, what I would propose is:

  1. Revert this change
  2. Add variables for repository_username and repository_password and pass those down to the helm configs
  3. Update the example and documentation to call this out for folks. Show that they will need the data source and then how to pass its outputs into the module inputs. Also, show that they will need an aliased provider for us-east-1 specifically for these creds

Copy link

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

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

See my comments. Just add two variables in variables.tf file for ecr username and password. Also, adde a documentation or example to consume this module using dedicated provider.

@@ -6,6 +6,9 @@ data "aws_eks_cluster" "this" {
name = local.cluster_id
}

# Equivalent of aws ecr get-login
data "aws_ecrpublic_authorization_token" "token" {}

Choose a reason for hiding this comment

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

remove this data source

Comment on lines +63 to +64
repository_username = data.aws_ecrpublic_authorization_token.token.user_name
repository_password = data.aws_ecrpublic_authorization_token.token.password

Choose a reason for hiding this comment

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

repalce the data source values with ecr_username and ecr_passsword

Comment on lines +139 to +140
repository_username = data.aws_ecrpublic_authorization_token.token.user_name
repository_password = data.aws_ecrpublic_authorization_token.token.password

Choose a reason for hiding this comment

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

Same as above comment

Comment on lines +206 to +207
repository_username = data.aws_ecrpublic_authorization_token.token.user_name
repository_password = data.aws_ecrpublic_authorization_token.token.password

Choose a reason for hiding this comment

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

same as above comment

Comment on lines +273 to +274
repository_username = data.aws_ecrpublic_authorization_token.token.user_name
repository_password = data.aws_ecrpublic_authorization_token.token.password

Choose a reason for hiding this comment

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

same as above comment

Comment on lines +341 to +342
repository_username = data.aws_ecrpublic_authorization_token.token.user_name
repository_password = data.aws_ecrpublic_authorization_token.token.password

Choose a reason for hiding this comment

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

same as above comment

Comment on lines +409 to +410
repository_username = data.aws_ecrpublic_authorization_token.token.user_name
repository_password = data.aws_ecrpublic_authorization_token.token.password

Choose a reason for hiding this comment

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

same as above comment

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.

4 participants