Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

#745 environment variables to be used instead of 'region' property for aws-ecr registry #841

Merged
merged 1 commit into from
Nov 23, 2020

Conversation

quintok
Copy link
Contributor

@quintok quintok commented Nov 23, 2020

I thought I'd give this a go as it looked like a pretty simple change and a good "first issue"

A few thoughts:

  1. This logic should be centralised for all aws environment config.
  2. Packer and terraform use https://github.com/hashicorp/aws-sdk-go-base which seems reasonable to migrate to.
  3. This logic is too simple, it should try and lean on the SDK more (ie: load region from default profile)
  4. The logic about leaning on the sdk seems similar between terraform and packer - maybe we can push that into the aws-sdk-go-base?

Finally, my dev environment is not healthy enough to check the documentation output.
After installing protobuf from homebrew and running:
make gen/doc

./vendor/proto/api-common-protos/: warning: directory does not exist.
google/rpc/status.proto: File not found.
internal/server/proto/server.proto:10:1: Import "google/rpc/status.proto" was not found or had errors.
...

@hashicorp-cla
Copy link

hashicorp-cla commented Nov 23, 2020

CLA assistant check
All committers have signed the CLA.

@mitchellh
Copy link
Contributor

I’ll note that I do want to use the AWS SDK base lib we made but there was a reason we didn’t for 0.1 that I can’t recall. That would definitely be the more correct longer term solution.

Copy link
Member

@briancain briancain 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 this! Looks good to me 👍 We'll try to use the aws sdk in the future, but for now this looks good in the short term.

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

Successfully merging this pull request may close these issues.

4 participants