-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
added provisioned_throughput to instance_template and region_instance_template #11901
added provisioned_throughput to instance_template and region_instance_template #11901
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @SarahFrench, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Errors
|
Tests analyticsTotal tests: 0 Click here to see the affected service packages
Tests were added that are skipped in VCR:
View the build log |
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!
I can see there's a build error currently:
Error: google/services/compute/resource_compute_instance_template.go:1474:6: dc.provisionedThroughput undefined (type diskCharacteristics has no field or method provisionedThroughput)
It looks like you'll need to update the diskCharacteristics
struct defined in resource_compute_instance_template.go.tmpl
to have a new field called provisionedThroughput
. Given the diskCharacteristics structs returned from diskCharacteristicsFromMap are just used for comparisons I think that's the only change needed, but I've only taken a quick look.
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_instance_template" "primary" {
disk {
provisioned_throughput = # value needed
}
}
Resource: resource "google_compute_region_instance_template" "primary" {
disk {
provisioned_throughput = # value needed
}
}
|
Tests analyticsTotal tests: 1028 Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_instance_template" "primary" {
disk {
provisioned_throughput = # value needed
}
}
Resource: resource "google_compute_region_instance_template" "primary" {
disk {
provisioned_throughput = # value needed
}
}
|
1 similar comment
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_instance_template" "primary" {
disk {
provisioned_throughput = # value needed
}
}
Resource: resource "google_compute_region_instance_template" "primary" {
disk {
provisioned_throughput = # value needed
}
}
|
Tests analyticsTotal tests: 1028 Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Tests analyticsTotal tests: 1028 Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_region_instance_template" "primary" {
disk {
provisioned_throughput = # value needed
}
}
|
Tests analyticsTotal tests: 1028 Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
@SarahFrench Bunch of non-space characters somehow made their way in, but should be fine now :) |
@SarahFrench This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
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'm glad you were able to track down those non-space characters, how confusing! And using strings for comparing the value in diskCharacteristicsFromMap sounds like a good compromise.
Overall this looks good to approve, though I've suggested some additions to the field descriptions that come from the API docs. We could also add some validation to the fields in the schema, so Terraform gives users feedback if they try to set a bad value. This could be done using the validation.IntAtLeast helper function - what do you think?
mmv1/third_party/terraform/services/compute/resource_compute_instance_template.go.tmpl
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/compute/resource_compute_region_instance_template.go.tmpl
Outdated
Show resolved
Hide resolved
…nstance_template.go.tmpl Co-authored-by: Sarah French <[email protected]>
…egion_instance_template.go.tmpl Co-authored-by: Sarah French <[email protected]>
Thanks for the suggestion, looks good to me :) |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
1 similar comment
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 1028 Click here to see the affected service packages
🟢 All tests passed! View the build log |
Tests analyticsTotal tests: 1028 Click here to see the affected service packages
🟢 All tests passed! View the build log |
@lnesteroff @SarahFrench All good to merge? Would love to see these changes in! 🤩 |
There's something wrong with the checks, I'll re-run and merge : /gcbrun |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 1030 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
This PR is approved and has been waiting for merge for 1 week. Is it ready to merge? Use the label |
Send it!! 🎉 |
Added the option to configure provisioned_throughput for regional and global instance_templates similar to PR#8528. Both provisioned_iops and provisioned_throughput can be used to customise hyperdisks, so I also updated the wording of provisioned_iops so it isn't exclusively referring to PD extreme.
Release Note Template for Downstream PRs (will be copied)