Skip to content

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

Merged
merged 2 commits into from
May 27, 2020
Merged

Add InService option for AWS #39

merged 2 commits into from
May 27, 2020

Conversation

lucacome
Copy link
Contributor

Added in_service filter for AWS.

If set to true only the instances that are in the InService state of the Lifecycle will be added to the upstreams.

Copy link
Contributor

@Rulox Rulox left a 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

@lucacome
Copy link
Contributor Author

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.

@Rulox
Copy link
Contributor

Rulox commented May 20, 2020

Yeah unmarshall validation is enough then @lucacome good point :)

@lucacome lucacome force-pushed the filter-in-service branch from 7a31607 to b0b0616 Compare May 21, 2020 14:56
Copy link
Contributor

@pleshakov pleshakov left a 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

@pleshakov
Copy link
Contributor

Linking #37

@lucacome lucacome requested a review from pleshakov May 27, 2020 02:59
@lucacome lucacome force-pushed the filter-in-service branch from 3505e82 to c8284e3 Compare May 27, 2020 03:11
Copy link
Contributor

@pleshakov pleshakov left a 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 {
Copy link
Contributor

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?

@lucacome lucacome force-pushed the filter-in-service branch from 48081b6 to 234ab6a Compare May 27, 2020 23:19
@lucacome lucacome merged commit f65e197 into master May 27, 2020
@lucacome lucacome deleted the filter-in-service branch May 27, 2020 23:24
@lucacome lucacome added enhancement Pull requests for new features/feature enhancements minor labels Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants