-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Updated ASM terraform module for 1.8 and 1.9 #895
Conversation
Thanks for the PR! 🚀 |
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 for the PR @ameer00
default = "1.9" | ||
} | ||
|
||
variable "asm_git_tag" { |
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.
Do we really need to expose this level of detail? It's a bit concerning if customers are having to separately version the install script from ASM.
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.
Yes, this is a feedback directly from Lifecycle eng
default = "" | ||
} | ||
|
||
variable "mode" { |
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.
If a variable has only one acceptable value, it shouldn't be a variable.
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.
This will soon change to also support upgrade
default = "install" | ||
} | ||
|
||
variable "service_account" { |
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.
Do we need to use a service account to authenticate? I'd prefer if we use whatever credentials Terraform is running as (Application Default Credentials).
Downloading service account keys should not be encouraged.
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.
I agree. The install_asm script supports it as an option and this was a feedback from a partner.
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.
#905 created for follow up
…-modules#895) * updated asm module * updated asm module * updated asm module * updated asm module * updated asm module * updated asm module * updated asm module * updated asm module * updated asm module * updated asm module * updated asm module * updated asm module * updated asm module * updated asm module * updated asm module * updated asm module * updated asm module Co-authored-by: coder <[email protected]> Co-authored-by: Bharath KKB <[email protected]>
This is a major update to the current ASM terraform module with the following updates:
install
andupgrade
mode (upgrades supports deploying the new control plane only).install_asm
script.skip_validation
for CI systems where you dont want the script to perform validation checks.install_asm
script (for example1.8.2-asm.2+config2
).