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 tags and workflow_tags fields to google_workflows_workflow resource. Fixes b/374990996 #12085

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

BENY4M1N
Copy link
Member

@BENY4M1N BENY4M1N commented Oct 22, 2024

workflows: added `tags` and `workflow_tags` fields to `google_workflows_workflow` resource

Copy link

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

@SarahFrench, 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 ( 4 files changed, 182 insertions(+))
google-beta provider: Diff ( 4 files changed, 182 insertions(+))
terraform-google-conversion: Diff ( 1 file changed, 17 insertions(+))
Open in Cloud Shell: Diff ( 4 files changed, 156 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 4
Passed tests: 2
Skipped tests: 0
Affected tests: 2

Click here to see the affected service packages
  • workflows

Action taken

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

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccWorkflowsWorkflow_Update [Debug log]
TestAccWorkflowsWorkflow_workflowTagsExample [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
Contributor

@SarahFrench SarahFrench left a comment

Choose a reason for hiding this comment

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

👋🏻 Hi! Thanks for your PR

I cannot access b/374990996 as I'm a HashiCorp employee, and similarly I can't access any information about the tags field as it's not yet in the public documentation.

However, in the acceptance test debug logs I can see that the new field is accepted by the API when creating the resource and isn't readable from the API after creating, which matches how you've implemented the field.

Overall I'm happy to approve this PR, but I'm just flagging that the review is done on a best-effort basis.

@BENY4M1N One question I have before approving is: does merging this PR to main need to be timed with anything, or should I just go ahead?

@BENY4M1N
Copy link
Member Author

👋🏻 Hi! Thanks for your PR

I cannot access b/374990996 as I'm a HashiCorp employee, and similarly I can't access any information about the tags field as it's not yet in the public documentation.

However, in the acceptance test debug logs I can see that the new field is accepted by the API when creating the resource and isn't readable from the API after creating, which matches how you've implemented the field.

Overall I'm happy to approve this PR, but I'm just flagging that the review is done on a best-effort basis.

@BENY4M1N One question I have before approving is: does merging this PR to main need to be timed with anything, or should I just go ahead?

Hi Sarah,

Thanks for reviewing the PR.

Great question. This is my first terraform PR, so I am not really sure. Is merging this PR changes user facing docs such as https://registry.terraform.io/modules/GoogleCloudPlatform/cloud-workflows/google/latest? If so, this PR should be merged when the Workflow-Tags feature is launched (which is sometimes in December 2024). WDYT?

@SarahFrench
Copy link
Contributor

Great question. This is my first terraform PR, so I am not really sure. Is merging this PR changes user facing docs such as https://registry.terraform.io/modules/GoogleCloudPlatform/cloud-workflows/google/latest? If so, this PR should be merged when the Workflow-Tags feature is launched (which is sometimes in December 2024). WDYT?

No worries! Merging this PR now would mean it'll enter a release that'll be published in ~2 weeks time (code merged to main before 8pm PT on a Tuesday will be released the following Monday). When the release is published the new field will be in user-facing docs for this resource in the Registry and will be mentioned in our release notes in GitHub, so users may attempt to use the new field before December.

Sounds like saving this PR to be merged in December is the route forward. In that case could you please @-mention me on this PR in December to get my attention so I can merge the PR on the correct date?

@BENY4M1N
Copy link
Member Author

Great question. This is my first terraform PR, so I am not really sure. Is merging this PR changes user facing docs such as https://registry.terraform.io/modules/GoogleCloudPlatform/cloud-workflows/google/latest? If so, this PR should be merged when the Workflow-Tags feature is launched (which is sometimes in December 2024). WDYT?

No worries! Merging this PR now would mean it'll enter a release that'll be published in ~2 weeks time (code merged to main before 8pm PT on a Tuesday will be released the following Monday). When the release is published the new field will be in user-facing docs for this resource in the Registry and will be mentioned in our release notes in GitHub, so users may attempt to use the new field before December.

Sounds like saving this PR to be merged in December is the route forward. In that case could you please @-mention me on this PR in December to get my attention so I can merge the PR on the correct date?

Sounds good, I'll mention you in this PR, when we launched the feature.

Copy link
Contributor

@SarahFrench SarahFrench left a comment

Choose a reason for hiding this comment

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

Approving, but awaiting instruction to merge as per discussion above.

Copy link
Contributor

@wyardley wyardley left a comment

Choose a reason for hiding this comment

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

I'm not an employee of Google / Hashicorp, so just throwing up a tiny suggestion here in case it's useful (not sure if you prefer one vs. the other @SarahFrench), but I think some of the Terraform code in the examples can be slightly simplified and made more idiomatic (I've been adjusting some other examples that are done the other way to use this style, and seems to work), so just FWIW in case it's useful.

If that format (the terser one) is preferred, I can make a couple of PRs to adjust some of the other stuff in the codebase to use this format (which would basically look like this).

@BENY4M1N
Copy link
Member Author

I'm not an employee of Google / Hashicorp, so just throwing up a tiny suggestion here in case it's useful (not sure if you prefer one vs. the other @SarahFrench), but I think some of the Terraform code in the examples can be slightly simplified and made more idiomatic (I've been adjusting some other examples that are done the other way to use this style, and seems to work), so just FWIW in case it's useful.

If that format (the terser one) is preferred, I can make a couple of PRs to adjust some of the other stuff in the codebase to use this format (which would basically look like this).

Hi Will,

Thank you for proposing modifications to my PR. I actually recently found out that I have to add a deletion_protection field to our Terraform resource as well as tags field. I will add the field and apply your comment.

@github-actions github-actions bot requested a review from SarahFrench October 23, 2024 17:37
@wyardley
Copy link
Contributor

Side note: #12118 and #12132 have the updates to existing files.

@BENY4M1N BENY4M1N changed the base branch from main to purpose_computed October 30, 2024 19:28
@BENY4M1N BENY4M1N changed the base branch from purpose_computed to main October 30, 2024 19:28
@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 ( 4 files changed, 184 insertions(+))
google-beta provider: Diff ( 4 files changed, 184 insertions(+))
terraform-google-conversion: Diff ( 1 file changed, 17 insertions(+))
Open in Cloud Shell: Diff ( 4 files changed, 157 insertions(+))

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 ( 4 files changed, 184 insertions(+))
google-beta provider: Diff ( 4 files changed, 184 insertions(+))
terraform-google-conversion: Diff ( 1 file changed, 17 insertions(+))
Open in Cloud Shell: Diff ( 4 files changed, 157 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 6
Passed tests: 6
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages
  • workflows

🟢 All tests passed!

View the build log

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 6
Passed tests: 6
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages
  • workflows

🟢 All tests passed!

View the build log

@SarahFrench SarahFrench changed the title Add tags field to workflow resource. Fixes b/374990996 Add tags and workflow_tags fields to google_workflows_workflow resource. Fixes b/374990996 Nov 6, 2024
@SarahFrench
Copy link
Contributor

SarahFrench commented Nov 21, 2024

@BENY4M1N - Hi 👋🏻 Any PRs merged about now would be part of a release on December 9th. We have some releases skipped during December due to holidays etc.

Would being in the Dec 9th release work for you?

@BENY4M1N
Copy link
Member Author

@BENY4M1N - Hi 👋🏻 Any PRs merged about now would be part of a release on December 9th. We have some released skipped during December due to holidays etc.

Would being in the Dec 9th release work for you?

Hi @SarahFrench,

Thanks for letting me know. Is there any other release in December after 9th that this PR could be a part of? If it is, I will rather wait for the next release. Otherwise, let's merge this PR.

@SarahFrench
Copy link
Contributor

My understanding is that there are only going to be releases on December 9th and 16th - which is preferable?

@BENY4M1N
Copy link
Member Author

December 16 works best for us. Thanks!

@BENY4M1N
Copy link
Member Author

BENY4M1N commented Dec 6, 2024

@SarahFrench

Hey Sarah,

We have changed the launch date to the end of January so there is no need to merge this PR now. We should merge the PR, to ensure it is published after the end of Jan (in Feb).

Copy link

github-actions bot commented Dec 6, 2024

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

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

@github-actions github-actions bot requested a review from NickElliot December 6, 2024 14:19
@SarahFrench
Copy link
Contributor

Thanks for the update! I've left the Google provider project so I won't be the person who merges this PR. I see that the repo's automation has assigned another reviewer, so please coordinate with them in future 👋

@SarahFrench SarahFrench removed their request for review December 6, 2024 17:19
@SarahFrench
Copy link
Contributor

@NickElliot - I'm going to remove disable-review-reminders to make sure this PR doesn't slip through the cracks, but I imagine you'd want to re-add the label in future

Copy link
Contributor

@NickElliot NickElliot left a comment

Choose a reason for hiding this comment

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

@BENY4M1N can you go ahead and send me a release date target in chat or email so I can pin a reminder on my calendar? Thanks

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.

5 participants