-
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 Logging Settings datasources. #9526
Add Logging Settings datasources. #9526
Conversation
Hello! I am a robot. It looks like you are a: @rileykarson, 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. |
Tests analyticsTotal tests: 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 testsTestAccLoggingFolderSettings_datasource|TestAccLoggingProjectSettings_datasource |
Rerun these tests in REPLAYING mode to catch issues
|
Any insight to why |
Good question! I'm not entirely sure- I thought we might have been missing an env var & skipped the test, but that doesn't seem to be the case. @shuyama1 can you tell? |
Looks like it got passed in the replaying mode. I wonder if we run the actual acceptance test instead of in VCR during the replaying mode for this test specifically. I'll need to dig deep into it to find out what happens exactly. Meanwhile, I triggered an independent run for |
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.
We'll need to add website files as well: https://googlecloudplatform.github.io/magic-modules/develop/add-handwritten-datasource/#add-documentation.
Otherwise, generally looks good- one comment inline.
mmv1/third_party/terraform/services/logging/data_source_google_logging_folder_settings.go
Outdated
Show resolved
Hide resolved
d7575cb
to
d8fb8f4
Compare
Tests analyticsTotal tests:
|
Tests analyticsTotal tests:
|
Thanks @rileykarson I've fixed up the property and added documentation. |
mmv1/third_party/terraform/website/docs/d/logging_folder_settings.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/d/logging_organization_settings.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/d/logging_project_settings.html.markdown
Outdated
Show resolved
Hide resolved
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- will merge once the downstreams regenerate from the suggestions I applied.
(if I haven't by EOD feel free to bump / ping)
Tests analyticsTotal tests: 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 testsTestAccLoggingProjectSink_updatePreservesCustomWriter |
|
Yeah, that's non-blocking. Confirmed we're not merging a new cassette for that. |
Co-authored-by: Riley Karson <rileykarson@google.com>
Release Note Template for Downstream PRs (will be copied)