-
Notifications
You must be signed in to change notification settings - Fork 115
Add all valid regions as of July 19th 2018... #62
Conversation
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.
Looks good! Couple of questions, and it would be great if you could squash to eliminate some of the debugging commits.
cmd/root.go
Outdated
} | ||
if (! foundRegion) { |
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.
Can you run this through gofmt?
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.
Sure, will.
docker/main.go
Outdated
@@ -46,7 +46,7 @@ func (repository *Repository) Login(username, password string) { | |||
|
|||
func (repository *Repository) Build(tag string) { | |||
console.Debug("Building Docker image [%s]", repository.UriFor(tag)) | |||
console.Shell("docker build --tag %s .", repository.UriFor(tag)) | |||
console.Shell("docker build --rm=false --tag %s .", repository.UriFor(tag)) |
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.
Hmm - this seems unrelated to the fix proposed. Happy to merge it separately with some explanation about the benefits.
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.
Yes, I will put it into separate branch. Shortens build time when you frequently rebuild the same image.
Please keep in mind that there seems to be more changes required, as pointed out in #54 (issues regarding security groups). |
Thanks, @tisba. I suppose we can handle that separately. It seems possible to me off the top of my head that default VPCs have some kind of behavior difference between US East (N. Virginia) and elsewhere, though I don't know of any off the top of my head. |
I don't recall exactly what the issue was. The only solution I found back then was to provide an explicit security group. ¯_(ツ)_/¯ |
Also fixed failing loop that allows only one region ever.
Hi John,
|
I had the same issue after I removed default security group (which is open
to all) in order to tighten security.
…On Tue, Sep 4, 2018 at 6:53 PM Sebastian Cohnen ***@***.***> wrote:
I don't recall exactly what the issue was. The only solution I found back
then was to provide an explicit security group.
¯_(ツ)_/¯
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#62 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAp12XZ3xORWD3LcMLUez1u5wEfp8M9qks5uXrAIgaJpZM4WNs1e>
.
|
Are you planning to make a new release, @jpignata? |
please release 0.2.4 with this PR, thanks. |
Just to make it work for all non-US people out of the box...