-
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
Cloud Workstations - Workstation Config #7017
Cloud Workstations - Workstation Config #7017
Conversation
Oops! It looks like you're using an unknown release-note type in your changelog entries:
Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @SarahFrench, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
Oops! It looks like you're using an unknown release-note type in your changelog entries:
Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccLoggingBucketConfigProject_cmekSettings|TestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_withInvalidGatewayApiConfigChannel |
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. Terraform GA: Diff ( 2 files changed, 375 insertions(+)) |
Tests analyticsTotal tests: Errors occurred during REPLAYING 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. Terraform GA: Diff ( 2 files changed, 361 insertions(+)) |
Tests analyticsTotal tests: Errors occurred during REPLAYING 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. Terraform GA: Diff ( 2 files changed, 361 insertions(+)) |
Heya @SarahFrench, I've addressed the comments :)! |
/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. Terraform GA: Diff ( 2 files changed, 611 insertions(+)) |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
I believe that I messed something up in the test, moved the updating of the displayName field in the test through the context now. Can we re-run the checks? @SarahFrench - Does 05ef367 seems correct to you? |
That commit looks ok to me and should run. Mutating the value in context feels like a bad idea generally, but in this case it's a very simple test so I think it's ok. |
/gcbrun |
I felt the same, at first I was injecting the value and interpolating it in the terraform code - but I saw a lot of tests that do it on a context level. So wasn't quite sure what the standard was. Good to know for future PRs 👍🏼 |
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. Terraform GA: Diff ( 2 files changed, 611 insertions(+)) |
I think for handwritten update tests the typical way is to define two separate functions (with names ending in But having said that, if you're making a very small change then making 1 reusable function instead of 2 makes sense. 🤷 In this case I think mutating context is ok but only because the test is so simple and the field being mutated isn't used for identifying resources via the API. |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccWorkstationsWorkstationConfig_displayName|TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample |
|
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.
Ok let's get this merged! :D
Good luck with implementing the 3rd resource for hashicorp/terraform-provider-google#12763
Woohoo! Amazing, thanks for the extensive review @SarahFrench - on to the last resource! |
* Add WorkstationConfig resource * indent correctly * add the workstationConfig * Add WorkstationConfig * Create workstation_config_basic.tf.erb * Create resource_workstations_workstation_config_test.go.erb * fix indent * remove duplicate displayName field * add `workstationClusterId` param * set host attribute block * set the `workstation_cluster_id` * fix create * use a default for the container property * add input true * set api defaults * set `host.gceInstance.bootDiskSizeGb` to be computed * fix machine_type attribute * fix typo's * lower case funcs * Update mmv1/third_party/terraform/tests/resource_workstations_workstation_config_test.go.erb Co-authored-by: Riley Karson <rileykarson@google.com> * add location * add skip test to examples * add tests to run sequentially instead of paralell * set `<% unless version == 'ga' -%>` * Remove extra tests * align google_workstations_workstation_config resource in update test * Revert "align google_workstations_workstation_config resource in update test" This reverts commit 37a4af3. * Revert "Remove extra tests" This reverts commit 1fc0744. * revert skip test * align tests and remove update label * add encryptionKey attribute * indent the description * add encryption key test * Use a static `account_id` for KMS SA * address comments on pr * set some extra defaults * add 2 new tests to test host and persistent_directories * Move back up * have a single set of unless/end * remove data google project * Remove faulty import * remove duplicate output and fmt * remove output field on displayName * add displayName update test * Fix update of display name --------- Co-authored-by: Riley Karson <rileykarson@google.com>
* main: (41 commits) update the test cases to resolve resourcename not found error Adds `grpc` field to `liveness_probe` and `startup_probe` to `google_cloud_run_v2_service` resource (GoogleCloudPlatform#6987) Upgrade DCL to v1.34 (GoogleCloudPlatform#7276) Add max_distance field to group placement policy (GoogleCloudPlatform#7354) Add stateful_ips to region_per_instance_config and per_instance_config (GoogleCloudPlatform#7316) Added support for workload-vulnerability-scanning and workload-config-audit (GoogleCloudPlatform#7310) datacatalog - bump Taxonomy and PolicyTag to ga (GoogleCloudPlatform#6989) Added best practices documentation for ForceNew fields (GoogleCloudPlatform#7127) Split resources in "B" products (GoogleCloudPlatform#7350) force recreate on master_config.num_instances (GoogleCloudPlatform#7349) Fix DataFusion instance versions used in tests (GoogleCloudPlatform#7343) remove duplicate word in Cluster.yaml (GoogleCloudPlatform#7347) Move more billing tests that require permissions beyond Billing User to master billing account (GoogleCloudPlatform#7344) Remove artifact repository beta URL, fixup handwritten tests (GoogleCloudPlatform#7345) Cloud Workstations - Workstation Config (GoogleCloudPlatform#7017) Add missing `type` argument to data source docs (GoogleCloudPlatform#7341) Fix caps in spanner resource schema accesses (GoogleCloudPlatform#7346) Downgrade Go to 1.18, modify comments (GoogleCloudPlatform#7339) feat: Add support for deletion_policy on shared vpc service project (GoogleCloudPlatform#7283) fixed virtual field update issues (GoogleCloudPlatform#7318) ...
* Add WorkstationConfig resource * indent correctly * add the workstationConfig * Add WorkstationConfig * Create workstation_config_basic.tf.erb * Create resource_workstations_workstation_config_test.go.erb * fix indent * remove duplicate displayName field * add `workstationClusterId` param * set host attribute block * set the `workstation_cluster_id` * fix create * use a default for the container property * add input true * set api defaults * set `host.gceInstance.bootDiskSizeGb` to be computed * fix machine_type attribute * fix typo's * lower case funcs * Update mmv1/third_party/terraform/tests/resource_workstations_workstation_config_test.go.erb Co-authored-by: Riley Karson <rileykarson@google.com> * add location * add skip test to examples * add tests to run sequentially instead of paralell * set `<% unless version == 'ga' -%>` * Remove extra tests * align google_workstations_workstation_config resource in update test * Revert "align google_workstations_workstation_config resource in update test" This reverts commit 37a4af3. * Revert "Remove extra tests" This reverts commit 1fc0744. * revert skip test * align tests and remove update label * add encryptionKey attribute * indent the description * add encryption key test * Use a static `account_id` for KMS SA * address comments on pr * set some extra defaults * add 2 new tests to test host and persistent_directories * Move back up * have a single set of unless/end * remove data google project * Remove faulty import * remove duplicate output and fmt * remove output field on displayName * add displayName update test * Fix update of display name --------- Co-authored-by: Riley Karson <rileykarson@google.com>
The leading pull request for this is: #7005, that should be merged first. (as I've split up the PRs for this new service)
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)