-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add Container Registry Integration Tests for AWS ECR #502
Conversation
010841c
to
043fd30
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
11 similar comments
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
96c2683
to
c25a970
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
region := os.Getenv("AWS_REGION") | ||
if region == "" { | ||
t.Skipf("Skipping test due to missing AWS_REGION environment variable") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we let this be environmentally controlled, instead of just hard coding it to a specific zone?
I can't see a scenario where we will want to change the zone our tests run in, but I might be missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I see what you mean - the only thing I can think of is some sort of lazy failsafe that tries to capture both user error when running tests locally and CI failing to behave properly/regional AWS outage.
I'm not against just hard coding the region right here; but if that's a pattern we want to clean up we should probably do it across more than just this provider.
Fixes #487.
Adds integration tests for AWS ECR in node, python, go, and dotnet.
Removes extraneous duplicate test
Removes extraneous JS test