-
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
Add DatabaseConfig and WebServerConfig to Cloud Composer's EnvironmentConfig #3863
Add DatabaseConfig and WebServerConfig to Cloud Composer's EnvironmentConfig #3863
Conversation
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. Thanks for your contribution! A human will be with you soon. @rileykarson, please review this PR or find an appropriate assignee. |
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.
@anitakar: I see you've marked this as WIP- let me know if you'd like me to make a review pass or if you'd prefer to wait.
Also, if you're removing the WIP marking do you mind commenting? I'm not sure I'll get notified for a push or rename, so that's the most reliable method to make sure I see that the status has changed.
Sure, I will comment when the PR is ready for review. I was not sure whether some internal work has not started, so I created PR to avoid double work on the issue. |
2b54896
to
80a4e8b
Compare
80a4e8b
to
5351b2e
Compare
cdefe99
to
3850316
Compare
1 similar comment
3850316
to
72df7b3
Compare
Acceptance tests have passed successfully. |
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.
Generally looks good! I left a few comments about how we can guard against some unintuitive behaviours Terraform has with configs where fields in a block are left unspecified & a handful of places where spaces and tabs got messed up. (We haven't found a good solution for linting templated go files for this kind of stuff, sadly)
I started a test run in our CI as well, but it'll take a while (1-2 hours) to get back to me, so I'm responding before that's done.
third_party/terraform/resources/resource_composer_environment.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_composer_environment.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_composer_environment.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_composer_environment.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_composer_environment.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_composer_environment_test.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_composer_environment_test.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_composer_environment.go.erb
Outdated
Show resolved
Hide resolved
7c7049c
to
dc6443d
Compare
1 similar comment
dc6443d
to
288c4e8
Compare
288c4e8
to
aa12306
Compare
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.
Looking great- just a few small documentation changes to reflect the fact that the subfields are required & can't be left unspecified.
third_party/terraform/website/docs/r/composer_environment.html.markdown
Outdated
Show resolved
Hide resolved
third_party/terraform/website/docs/r/composer_environment.html.markdown
Outdated
Show resolved
Hide resolved
third_party/terraform/website/docs/r/composer_environment.html.markdown
Outdated
Show resolved
Hide resolved
third_party/terraform/website/docs/r/composer_environment.html.markdown
Outdated
Show resolved
Hide resolved
1 similar comment
e9b277d
to
25ca0ec
Compare
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- sorry for the delay in getting back! Busy end of last week.
I had some composer tests fail in CI, but I think they're flaky and I don't believe it was due to this PR. I'll veryify them- they take a while to run, so it might just take me a bit between LGTM-ing this and clicking merge.
We're good- all tests passed, it was just an API flake before. |
Adds new sections to Cloud Composer's EnvironmentConfig used for environment creation and updating
See: https://cloud.google.com/composer/docs/reference/rest/v1beta1/projects.locations.environments#EnvironmentConfig
fixes {https://github.com/hashicorp/terraform-provider-google/issues/6916}
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)