-
Notifications
You must be signed in to change notification settings - Fork 26
Add InService option for AWS #39
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
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.
lgtm. I asked a question, feel free to ignore if it doesn't make sense.
Additionally, can we add validation to the new field as well with b, err := strconv.ParseBool("true")
? We validate all the fields of the config file here: https://github.com/nginxinc/nginx-asg-sync/blob/master/cmd/sync/aws.go#L184
I wasn't sure about adding validation, because a wrong value will throw an error on unmarshal and never reach validation, but I can add that. |
Yeah unmarshall validation is enough then @lucacome good point :) |
7a31607
to
b0b0616
Compare
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.
@lucacome Please see my comments
Linking #37 |
3505e82
to
c8284e3
Compare
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.
@lucacome looks good! approving
I left a few optional comments/suggestions.
|
||
batches := prepareBatches(maxItems, instanceIds) | ||
|
||
if len(batches) > len(ids)/maxItems+1 { |
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.
because we have a specific input here, perhaps it makes sense to use a number, like len(batches) != 2
, so that is is easier to read the test?
48081b6
to
234ab6a
Compare
Added
in_service
filter for AWS.If set to
true
only the instances that are in theInService
state of the Lifecycle will be added to the upstreams.