-
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 #7018
Cloud Workstations - Workstation #7018
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. @rileykarson, 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. |
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 testsTestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_withInvalidGatewayApiConfigChannel|TestAccLoggingBucketConfigProject_cmekSettings|TestAccBigQueryDataTable_bigtable |
Tests passed during RECORDING mode: All tests passed |
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 ( 1 file changed, 130 insertions(+)) |
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|TestAccContainerCluster_failedCreation |
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 see this is a draft- let me know if you'd prefer I don't comment any more until it's marked ready! There isn't standard etiquette around drafts I am aware of so I'm never sure if I should wait or not.
/gcbrun |
Pulliing this ready for review now :) |
Heya @trodge, just fixed the issue raised in the |
Just a heads up that some of the Workstation tests merged into main recently have failed - hashicorp/terraform-provider-google#13847 The failure above for TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample is:
I think this is due to the test now running across multiple PRs at once, versus running once at a time on the PR that added the test. My bad, I should have spotted during review! |
I think the test above can be fixed by adding the random suffix the end of |
I'll prep a PR for this @SarahFrench 👍🏼 |
This was just a 500, not a synchronous request timeout. Gonna check this was intermittent / generate new logs by trying again: /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 ( 4 files changed, 192 insertions(+), 2 deletions(-)) |
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. Terraform GA: Diff ( 4 files changed, 192 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccWorkstationsWorkstation_update|TestAccWorkstationsWorkstation_workstationBasicExample|TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample|TestAccFirebaserulesRelease_BasicRelease |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample|TestAccWorkstationsWorkstation_update|TestAccWorkstationsWorkstation_workstationBasicExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
The provider crashed while running the VCR tests in RECORDING mode |
(we hit a global timeout to cause that crash) |
Any idea what the error might indicate @rileykarson ? |
The previous error was just the timeout manifesting in a different way. /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 ( 4 files changed, 192 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccWorkstationsWorkstation_workstationBasicExample|TestAccWorkstationsWorkstation_update|TestAccFirebaserulesRelease_BasicRelease|TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample |
Heading off to bed, if there's anything that needs to be fixed - feel free to commit in my branch, otherwise I'll address it tomororow morning 👌 |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Rebasing, should solve the failed test. Other tests now passed too - great. On phone now, can't rebase.. |
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
Merging does the same thing as a rebase anyways, there's no need to do one unless there are merge conflicts or you're asked to by a reviewer!
Amazing! I'll create another PR for the IAM resource tomorrow :) |
Fixes hashicorp/terraform-provider-google#12763
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)