Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat!: autoscaler supporting stateful ips #297

Merged
merged 11 commits into from
May 3, 2023
Merged

feat!: autoscaler supporting stateful ips #297

merged 11 commits into from
May 3, 2023

Conversation

ericyz
Copy link
Contributor

@ericyz ericyz commented Mar 2, 2023 •
edited
Loading

Issues: #280

@ericyz ericyz requested a review from a team as a code owner March 2, 2023 23:53
@ericyz ericyz force-pushed the feat/autoscalerstateful-ip branch from c31b056 to 2dd9851 Compare March 2, 2023 23:56
@ericyz ericyz mentioned this pull request Mar 2, 2023
Closed
@ericyz ericyz force-pushed the feat/autoscalerstateful-ip branch 2 times, most recently from 9fba0ff to 75b2ee3 Compare March 3, 2023 00:20
@cydergoth
Copy link

Thank you!

@ericyz ericyz force-pushed the feat/autoscalerstateful-ip branch 7 times, most recently from ebdc85e to 51aad92 Compare March 3, 2023 03:27
@ericyz ericyz changed the title feat: autoscaler supporting stateful ips feat!: autoscaler supporting stateful ips Mar 3, 2023
@ericyz ericyz force-pushed the feat/autoscalerstateful-ip branch 2 times, most recently from 6e1917d to 9e3ae3e Compare March 5, 2023 23:21
@ericyz
Copy link
Contributor Author

ericyz commented Mar 5, 2023

/gcbrun

@ericyz ericyz force-pushed the feat/autoscalerstateful-ip branch 6 times, most recently from f5f157b to aff958c Compare March 6, 2023 06:53
@ericyz ericyz force-pushed the feat/autoscalerstateful-ip branch from aff958c to e522392 Compare March 6, 2023 07:36
@ericyz
Copy link
Contributor Author

ericyz commented Mar 7, 2023

Hi @terraform-google-modules/cft-admins , please review

g-awmalik
g-awmalik reviewed Mar 7, 2023
View reviewed changes
Copy link
Contributor

@g-awmalik g-awmalik 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 the PR. Few comments.

autogen/variables.tf.tmpl Outdated Show resolved Hide resolved
autogen/main.tf.tmpl Show resolved Hide resolved
test/fixtures/mig/stateful/main.tf Outdated Show resolved Hide resolved
test/integration/mig_stateful/controls/mig_stateful.rb Outdated Show resolved Hide resolved
@ericyz ericyz force-pushed the feat/autoscalerstateful-ip branch 8 times, most recently from 39f8bd3 to cdcf03f Compare March 13, 2023 03:46
@ericyz ericyz force-pushed the feat/autoscalerstateful-ip branch 2 times, most recently from fdf1b0e to eac3935 Compare March 13, 2023 04:13
@ericyz ericyz force-pushed the feat/autoscalerstateful-ip branch from eac3935 to 7e66888 Compare March 13, 2023 04:27
@ericyz
Copy link
Contributor Author

ericyz commented Mar 13, 2023 •
edited
Loading

@g-awmalik - please help to review when you have time. Thanks

@ericyz ericyz requested a review from g-awmalik March 14, 2023 03:10
g-awmalik
g-awmalik approved these changes Mar 17, 2023
View reviewed changes
Copy link
Contributor

@g-awmalik g-awmalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good for the most part. Please review a few more comments. Thanks.

.kitchen.yml Outdated Show resolved Hide resolved
build/int.cloudbuild.yaml Outdated
waitFor:
- create-all
name: 'gcr.io/cloud-foundation-cicd/$_DOCKER_IMAGE_DEVELOPER_TOOLS:$_DOCKER_TAG_VERSION_DEVELOPER_TOOLS'
args: ['/bin/bash', '-c', 'cd test/integration && RUN_STAGE=init go test -v -run TestMigStatefulModule ./... -p 1']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified as:
cft test run TestMigStatefulModule --stage apply --verbose

And all throughout...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Updating it

examples/mig_stateful/main.tf Outdated Show resolved Hide resolved
examples/mig_stateful/main.tf
Comment on lines +70 to +80
update_policy = [{
max_surge_fixed = 0
instance_redistribution_type = "NONE"
max_surge_percent = null
max_unavailable_fixed = 4
max_unavailable_percent = null
min_ready_sec = 180
replacement_method = "RECREATE"
minimal_action = "REFRESH"
type = "OPPORTUNISTIC"
}]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any assertions for this in your test. I would suggest either take these (and other) superfluous configs out of the example, if they are not critical for the example OR add assertions in your test if they are necessary.

@ericyz ericyz force-pushed the feat/autoscalerstateful-ip branch from 5d88abd to a97c11b Compare May 3, 2023 02:56
@ericyz ericyz force-pushed the feat/autoscalerstateful-ip branch from cbe990a to 2ef9a3c Compare May 3, 2023 03:49
@ericyz ericyz merged commit 5d13e8b into terraform-google-modules:master May 3, 2023
@ericyz ericyz deleted the feat/autoscalerstateful-ip branch May 3, 2023 06:14
@release-please release-please bot mentioned this pull request May 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@g-awmalik g-awmalik g-awmalik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ericyz @cydergoth @g-awmalik