-
Notifications
You must be signed in to change notification settings - Fork 459
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
feat: allow for configuring auto_scaler_profile #278
feat: allow for configuring auto_scaler_profile #278
Conversation
@microsoft-github-policy-service agree |
c385e54
to
a78909e
Compare
Hello, thanks for your contribution @davidspek Could you please run locally the https://github.com/Azure/terraform-azurerm-aks#pre-commit--pr-check--test I see this step is failing for this PR:
the variables in the thanks |
@zioproto Thanks for the quick response. I had run the commands locally but it seems like it didn't run quite correctly. Everything should be good now. |
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.
Hello @davidspek, thanks for opening this pr! Almost LGTM but we'd like to keep our variables' description and default value as same as the officical document, so would you please align the description and default value for us? Thanks for your understanding!
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.
Thanks @davidspek for your reply, we still need enum and number values in the description to be correctly backquoted, would you please copy the descriptions from this markdown document? Thanks for your understanding!
@lonegunmanb Sorry for the late response. I've made the changes you've requested. Regarding the |
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.
Thanks @davidspek for this update, we use terraform-docs to generate readme file from terraform code including this variables.tf
, so it's very important to keep variables' description
value to valid markdown format so the generated readme file's format could be correct. In the description
we should backquote the value. I would suggest that we just copy the provider's document in provider's repo.
@lonegunmanb Sorry for all the back and forth with all this. I just pushed a commit that I think resolves all your comments. |
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.
Hi @davidspek thanks for the update, almost LGTM and we've passed the e2e test. Only one change request, we can remove default = null
, after that, we're good to merge.
Thanks for your understanding and contribution!
@lonegunmanb Sorry for the late response, I was away on vacation. I'll remove the merge conflicts and make the last change shortly. |
5fa3faa
to
75accb6
Compare
@lonegunmanb If I remove the |
@lonegunmanb It seems like |
Hello @davidspek , I'm sorry we've met an issue on our pipeline so we have to upgrade the Github action yaml files in this repo, would you please merge your branch with the latest master branch so we can run the e2e test? Apology for the inconvenience, we're still refactoring the pipeline. |
d2d79c6
to
a727416
Compare
@lonegunmanb No problem. I've rebased on the latest master and pushed it now. |
Hello @davidspek , we have a version upgrade test failure:
If our users upgrade the module's version to the merged version then they would face such change. The issue was caused by reusing I would suggest adding a new variable as the toggle to turn on and off this dynamic block. |
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.
We need a new toggle variable for this new added dynamic block to avoid surprise change.
@lonegunmanb Sorry for the late reply, we were out of office during the holidays. Before I make the above change, would you otherwise prefer a single map variable with all the settings for the autoscaling profile, rather than a dedicated variable for each autoscaler profile setting? |
Good point! I'm doing such practices in new created module like this one by using But for aks module, my concern is we need keep consistency among our variables. Maybe someday we could publish a major release, converting variables into |
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.
Thanks @davidspek for updating this pr, only some tiny issues to solve would make this pr been merged.
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
Signed-off-by: David van der Spek <[email protected]>
3683dad
to
93bea23
Compare
Signed-off-by: David van der Spek <[email protected]>
@lonegunmanb Should be all good now. |
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.
Thanks @davidspek, LGMT! 🚀
Describe your changes
This PR allows for users to specify the
auto_scaler_profile
settings when autoscaling is enabled. Some of these settings are quite vital too having the autoscaler work nicely.Issue number
#000
Checklist before requesting a review
CHANGELOG.md
fileThanks for your cooperation!