data.aws_ecr_repository: Add new ECR Repository data source #944
Conversation
There was a problem hiding this comment.
Hi @RyanFrench,
Thanks for the work here, this is a nice addition! :)
I left a few comments regarding wording mainly. Could you give an eye on this please?
Thanks!
| Read: dataSourceAwsEcrRepositoryRead, | ||
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| "name": &schema.Schema{ |
There was a problem hiding this comment.
These redundant composite literals can be safely removed (doable since Go 1.7), so this could be:
"name": {
Could you reflect the same modification everywhere where needed? Thanks!
| conn := meta.(*AWSClient).ecrconn | ||
|
|
||
| repositoryName := d.Get("name").(string) | ||
| log.Printf("[DEBUG] Reading repository %s", repositoryName) |
There was a problem hiding this comment.
To better debug things, could you update this sentence to: Reading ECR repository named %s?
|
|
||
| repository := out.Repositories[0] | ||
|
|
||
| log.Printf("[DEBUG] Received repository %s", out) |
There was a problem hiding this comment.
Could you also update this log for something like: Received ECR repository %s?
Also, if you are exposing the out structure, I would suggest something like Received ECR repository %#v, what do you think?
|
|
||
| The following arguments are supported: | ||
|
|
||
| * `name` - (Required) The name of the ECR Repository |
There was a problem hiding this comment.
As for attributes, could you also add a dot at the end?
|
|
||
| * `arn` - Full ARN of the repository. | ||
| * `registry_id` - The registry ID where the repository was created. | ||
| * `repository_url` - The URL of the repository (in the form `aws_account_id.dkr.ecr.region.amazonaws.com/repositoryName` |
There was a problem hiding this comment.
Small typo there: missing bracket at the end
|
Hi @Ninir Thank you for the review. I believe I have now addressed your comments. |
There was a problem hiding this comment.
Hey @RyanFrench,
Just added a log instruction when the ECR repository is not found.
Just ran the test, this seems all good to me otherwise!
$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSEcrDataSource_ecrRepository'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEcrDataSource_ecrRepository -timeout 120m
=== RUN TestAccAWSEcrDataSource_ecrRepository
--- PASS: TestAccAWSEcrDataSource_ecrRepository (16.29s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 16.319s
Thanks for your work! :)
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
ghost
locked and limited conversation to collaborators
Adds a new data source, for retrieving information about an ECR repository using the name.