Skip to content
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 post_read custom_code #12703

Merged
merged 4 commits into from
Jan 7, 2025
Merged

Conversation

coder-221
Copy link
Member

@coder-221 coder-221 commented Jan 7, 2025

This is to unblock a future PR for etags in access_context_manager resources. We'll need to access the etag in the GET response that's made from the nested_query code. For consistency, I also added the post_read to the normal resource read function as well, but that's not strictly required for our use case.

I'm also not sure what the params false - are for in the template, so would appreciate a check to ensure those are correct. Thanks!

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.


@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 13 files changed, 13 deletions(-))
google-beta provider: Diff ( 13 files changed, 13 deletions(-))

1 similar comment
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 13 files changed, 13 deletions(-))
google-beta provider: Diff ( 13 files changed, 13 deletions(-))

@coder-221 coder-221 marked this pull request as ready for review January 7, 2025 19:27
@github-actions github-actions bot requested a review from c2thorn January 7, 2025 19:27
Copy link

github-actions bot commented Jan 7, 2025

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@c2thorn, 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.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 13 files changed, 13 deletions(-))
google-beta provider: Diff ( 13 files changed, 13 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 1208
Passed tests: 1125
Skipped tests: 83
Affected tests: 0

Click here to see the affected service packages
  • accesscontextmanager
  • bigquery
  • compute

🟢 All tests passed!

View the build log

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 1208
Passed tests: 1125
Skipped tests: 83
Affected tests: 0

Click here to see the affected service packages
  • bigquery
  • compute
  • accesscontextmanager

🟢 All tests passed!

View the build log

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 1208
Passed tests: 1124
Skipped tests: 83
Affected tests: 1

Click here to see the affected service packages
  • accesscontextmanager
  • bigquery
  • compute

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccComputeRegionPerInstanceConfig_removeInstanceOnDestroy

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccComputeRegionPerInstanceConfig_removeInstanceOnDestroy [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.


🟢 All tests passed!

View the build log or the debug log for each test

Copy link
Member

@c2thorn c2thorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide an example template and use it with post_read in some resource's YAML file so we can see that its working?

The template can just add dummy comments so it won't have any functional change to the resource

@@ -243,6 +243,9 @@ func resource{{ $.ResourceName }}ListForPatch(d *schema.ResourceData, meta inter
return nil, err
}

{{- if $.CustomCode.PostRead }}
{{ $.CustomTemplate $.CustomCode.PostRead false -}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the CustomTemplate function is from here: https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/api/resource.go#L1510

the false will toggle off the option for appending a new line - a feature we really only cared about for an older migration and should not be relevant either way for you

the - tells the go template to delete any whitespace after the statement. Getting an example working with this should show how that ends up looking

@github-actions github-actions bot requested a review from c2thorn January 7, 2025 20:44
@coder-221
Copy link
Member Author

Can you provide an example template and use it with post_read in some resource's YAML file so we can see that its working?

The template can just add dummy comments so it won't have any functional change to the resource

Done and thanks for the review! I added a post_read template to one of the ACM resources and checked the generated output. I can see both places where it inserted the code comments from the template and I think it looks correct

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 13 files changed, 7 insertions(+), 12 deletions(-))
google-beta provider: Diff ( 13 files changed, 7 insertions(+), 12 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 1208
Passed tests: 1125
Skipped tests: 83
Affected tests: 0

Click here to see the affected service packages
  • accesscontextmanager
  • bigquery
  • compute

🟢 All tests passed!

View the build log

Copy link
Member

@c2thorn c2thorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly what I wanted to see. Generated output looks good, so looks like post_read is good to go. Thanks @coder-221 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants