-
Notifications
You must be signed in to change notification settings - Fork 28
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!: Latest SOT sync of NW, HANA, and S4 templates. #140
Conversation
PiperOrigin-RevId: 507805426
PiperOrigin-RevId: 509831816
PiperOrigin-RevId: 523996003
PiperOrigin-RevId: 529167790
PiperOrigin-RevId: 530312596
PiperOrigin-RevId: 530708592
PiperOrigin-RevId: 530888107
PiperOrigin-RevId: 534931390
PiperOrigin-RevId: 536400676
PiperOrigin-RevId: 537023870
PiperOrigin-RevId: 537368325
PiperOrigin-RevId: 546061202
PiperOrigin-RevId: 546882520
PiperOrigin-RevId: 548962523
PiperOrigin-RevId: 550017353
PiperOrigin-RevId: 553138230
PiperOrigin-RevId: 564847003
PiperOrigin-RevId: 571022240
PiperOrigin-RevId: 574474234
PiperOrigin-RevId: 576166349
PiperOrigin-RevId: 576571991
PiperOrigin-RevId: 581319687
PiperOrigin-RevId: 582060672
PiperOrigin-RevId: 584040296
PiperOrigin-RevId: 588410908
PiperOrigin-RevId: 596578712
PiperOrigin-RevId: 607322441
PiperOrigin-RevId: 613223659
PiperOrigin-RevId: 620832891
PiperOrigin-RevId: 622211732
PiperOrigin-RevId: 627135326
PiperOrigin-RevId: 627710410
PiperOrigin-RevId: 631442037
PiperOrigin-RevId: 631518614
PiperOrigin-RevId: 632244710
… no longer in use.
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.
LGTM for the most part. I have a few suggestions but feel free to decide as you see fit.
@@ -0,0 +1,24 @@ | |||
# SAP HANA example | |||
|
|||
This example illustrates how to use the latest release of the terraform module for SAP on Google Cloud for provisioning SAP NW |
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 would highly recommend spelling out the acronyms here in the description.
zone = var.zone1_name | ||
} | ||
|
||
resource "google_project_iam_member" "ansible_sa_role_1" { |
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.
Have you considered this projects_iam module for setting IAMs for this SA? This can make the code a lot cleaner but will introduce a dependency on a module.
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.
At the moment making this change in TF would be difficult due to the build processes we use. So this could be useful in the future but for now can't be done.
type = "A" | ||
} | ||
|
||
resource "google_project_iam_member" "app_sa_role_1" { |
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.
Same here regarding the usage of the module I mentioned above.
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.
At the moment making this change in TF would be difficult due to the build processes we use. So this could be useful in the future but for now can't be done.
type = string | ||
} | ||
|
||
variable "package_location" { |
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.
Since you're parsing this with an index here, it'll be good to set a validation check here to make sure the input complies with a certain format.
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 took a ticket to add these validations in. Theres a few others that would make sense as well.
Also, can you make the PR a bit more descriptive since it appears in the release notes. Thanks! |
Latest changes for templates currently available in GCS.