-
Notifications
You must be signed in to change notification settings - Fork 35
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
Support bypass_platform_safety_checks_on_user_schedule_enabled
and reboot_setting
#66
Support bypass_platform_safety_checks_on_user_schedule_enabled
and reboot_setting
#66
Conversation
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
bypass_platform_safety_checks_on_user_schedule_enabled
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
bypass_platform_safety_checks_on_user_schedule_enabled
bypass_platform_safety_checks_on_user_schedule_enabled
and reboot_setting
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
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 @davidkarlsen for opening this pr! Some review comments.
thanks @lonegunmanb - I think I got all fixed now. |
@lonegunmanb if it passes and you like it - can you push a new tag? TIA |
@lonegunmanb wdyt? |
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 @davidkarlsen for the update. We prefer using Terraform conditional expression when either one of both sides could throw an error, like:
condition = reboot_setting == null ? true : contains(["Always", "IfRequired", "Never"], var.reboot_setting)
This is a good example, since when reboot_setting
is null
, contains(["Always", "IfRequired", "Never"], var.reboot_setting)
could throw an error, and since Terraform's logical binary operators are not short-circuit, we have to use a conditional expression to avoid error.
But once there won't be error thrown by the expression, we still prefer a simpler version, like some comments I'v left.
Again, thanks for your pr! We cannot maintain these modules without your support! Thanks @davidkarlsen !
Sure thing, I can publish a new version for this pr. @davidkarlsen |
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 @davidkarlsen we have another syntax error needs to be fixed. Would you please check pre-comment document and run the check before you commit? Thanks!
done |
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 @davidkarlsen thanks for the update, we still have a syntax error. You can read the document and run make pr-check
to verify the pr, thanks!
When you update the pr please feel free to ping me here, I'll review and merge this pr ASAP.
docker run --rm -v $(pwd):/src -w /src mcr.microsoft.com/azterraform:latest make pre-commit &> log
cat log|grep -v "downloading "
terraform-azurerm-virtual-machine cat log|grep -v "downloading "
==> Formatting terraform code...
terraform fmt -recursive
==> Fixing test and document terraform blocks code with terrafmt...
time="2023-11-22 10:26:58" level=error msg="block 3 @ ./README.md:93 failed to process with: failed to parse hcl: ./README.md:2,26-27: Invalid expression; Expected the start of an expression, but found an invalid expression token.\nmodule \"example\" {\n source = <module_source>\n ...\n tracing_tags_enabled = true\n}\n"
time="2023-11-22 10:26:58" level=error msg="block 4 @ ./README.md:105 failed to process with: failed to parse hcl: ./README.md:2,25-26: Invalid expression; Expected the start of an expression, but found an invalid expression token.\nmodule \"example\" {\n source = <module_source>\n ...\n tracing_tags_prefix = \"custom_prefix_\"\n}\n"
==> Fixing source code with gofmt...
# This logic should match the search logic in scripts/gofmtcheck.sh
find . -name '*.go' | grep -v vendor | xargs gofmt -s -w
==> Fixing source code with Gofumpt...
# This logic should match the search logic in scripts/gofmtcheck.sh
find . -name '*.go' | grep -v vendor | xargs gofumpt -w
--> Generating doc
README.md updated successfully |
This seems to fail due to infra: performing CreateOrUpdate: unexpected status 409 with error: OperationNotAllowed: Operation could not be completed as it results in exceeding approved standardFSv2Family Cores quota. |
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 @davidkarlsen, LGTM!
Describe your changes
Adds support for
bypass_platform_safety_checks_on_user_schedule_enabled
as well asreboot_setting
Issue number
#65
Checklist before requesting a review
CHANGELOG.md
fileThanks for your cooperation!