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.

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

Merged
merged 19 commits into from
Nov 23, 2023

Conversation

davidkarlsen
Copy link
Contributor

@davidkarlsen davidkarlsen commented Nov 16, 2023

Describe your changes

Adds support for bypass_platform_safety_checks_on_user_schedule_enabled as well as reboot_setting

Issue number

#65

Checklist before requesting a review

  • The pr title can be used to describe what this pr did in CHANGELOG.md file
  • I have executed pre-commit on my machine
  • I have passed pr-check on my machine

Thanks for your cooperation!

@davidkarlsen davidkarlsen changed the title Support bypass_platform_safety_checks_on_user_schedule_enabled Support bypass_platform_safety_checks_on_user_schedule_enabled Nov 16, 2023
Signed-off-by: David J. M. Karlsen <[email protected]>
@davidkarlsen davidkarlsen changed the title Support bypass_platform_safety_checks_on_user_schedule_enabled Support bypass_platform_safety_checks_on_user_schedule_enabled and reboot_setting Nov 16, 2023
Signed-off-by: David J. M. Karlsen <[email protected]>
Signed-off-by: David J. M. Karlsen <[email protected]>
Copy link
Member

@lonegunmanb lonegunmanb left a 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.

variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
locals.tf Outdated Show resolved Hide resolved
@davidkarlsen
Copy link
Contributor Author

thanks @lonegunmanb - I think I got all fixed now.

@davidkarlsen
Copy link
Contributor Author

@lonegunmanb if it passes and you like it - can you push a new tag? TIA

@davidkarlsen
Copy link
Contributor Author

@lonegunmanb wdyt?

Copy link
Member

@lonegunmanb lonegunmanb left a 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 !

variables.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
variables.tf Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
@lonegunmanb
Copy link
Member

@lonegunmanb if it passes and you like it - can you push a new tag? TIA

Sure thing, I can publish a new version for this pr. @davidkarlsen

Copy link
Member

@lonegunmanb lonegunmanb left a 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!

variables.tf Outdated Show resolved Hide resolved
@davidkarlsen
Copy link
Contributor Author

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

Copy link
Member

@lonegunmanb lonegunmanb left a 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.

variables.tf Outdated Show resolved Hide resolved
@davidkarlsen
Copy link
Contributor Author

@lonegunmanb

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

@davidkarlsen
Copy link
Contributor Author

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.

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @davidkarlsen, LGTM!

@lonegunmanb lonegunmanb merged commit b64ecfc into Azure:main Nov 23, 2023
4 checks passed
@davidkarlsen davidkarlsen deleted the feature/bypass_platform_safety branch December 21, 2023 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants