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

Remove relay_mode field support #10274

Conversation

siwiutki
Copy link
Contributor

@siwiutki siwiutki commented Mar 25, 2024

fixes hashicorp/terraform-provider-google#17997

relay_mode field from AdvancedDatapathObservabilityConfig is superseded by enable_relay field and will be deprecated on the provider side in near future.

This is a breaking change according to TF docs and as such, should be released in the next major release. Deprecation of relay_mode field on TF side has already happened more than a month ago, foreshadowing this change through deprecation message.

This field is part of a provider feature that is still in Public Preview and yet to go GA.

container: removed deprecated field `advanced_datapath_observability_config.relay_mode` from `google_container_cluster` resource. Users are expected to use `enable_relay` field instead.
container: made field `advanced_datapath_observability_config.enable_relay` required in `google_container_cluster` resource

@github-actions github-actions bot requested a review from zli82016 March 25, 2024 12:35
Copy link

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

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

@zli82016
Copy link
Member

/gcbrun

@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 ( 2 files changed, 7 insertions(+), 166 deletions(-))
google-beta provider: Diff ( 2 files changed, 7 insertions(+), 166 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

  • Field monitoring_config.advanced_datapath_observability_config.enable_relay changed from optional to required on google_container_cluster - reference
  • Field monitoring_config.advanced_datapath_observability_config.enable_relay default value changed from false to on google_container_cluster - reference
  • Field monitoring_config.advanced_datapath_observability_config.relay_mode within resource google_container_cluster was either removed or renamed - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

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 ( 2 files changed, 7 insertions(+), 166 deletions(-))
google-beta provider: Diff ( 2 files changed, 7 insertions(+), 166 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

  • Field monitoring_config.advanced_datapath_observability_config.enable_relay changed from optional to required on google_container_cluster - reference
  • Field monitoring_config.advanced_datapath_observability_config.enable_relay default value changed from false to on google_container_cluster - reference
  • Field monitoring_config.advanced_datapath_observability_config.relay_mode within resource google_container_cluster was either removed or renamed - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 185
Passed tests: 171
Skipped tests: 12
Affected tests: 2

Click here to see the affected service packages
  • container

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
TestAccContainerCluster_withMonitoringConfig|TestAccContainerCluster_withMonitoringConfigAdvancedDatapathObservabilityConfig

Get to know how VCR tests work

1 similar comment
@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 185
Passed tests: 171
Skipped tests: 12
Affected tests: 2

Click here to see the affected service packages
  • container

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
TestAccContainerCluster_withMonitoringConfig|TestAccContainerCluster_withMonitoringConfigAdvancedDatapathObservabilityConfig

Get to know how VCR tests work

@zli82016
Copy link
Member

The breaking change should be put in the next major release 6.0. The target date for major release is Aug 2024. It is a little bit earlier to work on release 6.0 now. You can close this PR and then open a new PR later for release 6.0.

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerCluster_withMonitoringConfig[Debug log]
TestAccContainerCluster_withMonitoringConfigAdvancedDatapathObservabilityConfig[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerCluster_withMonitoringConfig[Debug log]
TestAccContainerCluster_withMonitoringConfigAdvancedDatapathObservabilityConfig[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

@zli82016
Copy link
Member

zli82016 commented May 1, 2024

The change needs to be made for 6.0 release.

@zli82016 zli82016 closed this May 1, 2024
@c2thorn
Copy link
Member

c2thorn commented May 1, 2024

We can keep it open and move it to the eventual major release branch when it gets closer to time?

@zli82016 zli82016 reopened this May 1, 2024
@zli82016
Copy link
Member

zli82016 commented May 1, 2024

Created a Github issue hashicorp/terraform-provider-google#17997

@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 ( 2 files changed, 7 insertions(+), 166 deletions(-))
google-beta provider: Diff ( 2 files changed, 7 insertions(+), 166 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

  • Field monitoring_config.advanced_datapath_observability_config.enable_relay changed from optional to required on google_container_cluster - reference
  • Field monitoring_config.advanced_datapath_observability_config.enable_relay default value changed from false to on google_container_cluster - reference
  • Field monitoring_config.advanced_datapath_observability_config.relay_mode within resource google_container_cluster was either removed or renamed - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 190
Passed tests: 180
Skipped tests: 10
Affected tests: 0

Click here to see the affected service packages
  • container

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log

Copy link

@GoogleCloudPlatform/terraform-team This PR has been waiting for review for 3 weeks. Please take a look! Use the label disable-review-reminders to disable these notifications.

@zli82016
Copy link
Member

zli82016 commented Jul 9, 2024

@siwiutki , the window to contribute to the provider 6.0 release is open now. Feel free to move this PR to https://github.com/GoogleCloudPlatform/magic-modules/commits/FEATURE-BRANCH-major-release-6.0.0/

@siwiutki siwiutki changed the base branch from main to FEATURE-BRANCH-major-release-6.0.0 July 10, 2024 09:23
@siwiutki
Copy link
Contributor Author

@zli82016 I changed the target branch. Let me know if any more action is required from my side to push this forward.

@zli82016 zli82016 added override-breaking-change Allows a potential breaking change to be merged and removed disable-review-reminders labels Jul 10, 2024
@zli82016
Copy link
Member

zli82016 commented Jul 10, 2024

@siwiutki , thanks for making the change.

After checking the guide to make change in 6.0 release branch, more changes are needed.

  1. Add the deprecation message to relay_mode field in the doc container_cluster.html.markdown in main branch in a separate PR.
    Example can be found in the guide.
  2. Remove relay_mode field from the doc container_cluster.html.markdown in this PR.
  3. Add the upgrade guide entries to version_6_upgrade.html.markdown in this PR.

Copy link
Member

@zli82016 zli82016 left a comment

Choose a reason for hiding this comment

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

More changes are needed.

Copy link

@siwiutki, this PR is waiting for action from you. Please address any comments or change requests, or re-request review from a core reviewer if no action is required.

Image showing the re-request review button

If no action is taken, this PR will be closed in 28 days.

This notification can be disabled with the disable-automatic-closure label.

@siwiutki
Copy link
Contributor Author

  1. Add the deprecation message to relay_mode field in the doc container_cluster.html.markdown in main branch in a separate PR.

Since this requires a separate PR before we can handle this PR, let me mark this as draft for now so that reviewers won't be nagged or sth.

@siwiutki siwiutki marked this pull request as draft July 25, 2024 10:03
@siwiutki
Copy link
Contributor Author

@zli82016 Please let me know if there's anything more than #11254 that's required to handle your first requested point.

@zli82016
Copy link
Member

@zli82016 Please let me know if there's anything more than #11254 that's required to handle your first requested point.

That is it. Thanks for creating the PR #11254, which is merged to the main branch.

@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, 38 insertions(+), 171 deletions(-))
google-beta provider: Diff ( 4 files changed, 38 insertions(+), 171 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

  • Field monitoring_config.advanced_datapath_observability_config.enable_relay changed from optional to required on google_container_cluster - reference
  • Field monitoring_config.advanced_datapath_observability_config.enable_relay default value changed from false to on google_container_cluster - reference
  • Field monitoring_config.advanced_datapath_observability_config.relay_mode within resource google_container_cluster was either removed or renamed - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 198
Passed tests: 187
Skipped tests: 10
Affected tests: 1

Click here to see the affected service packages
  • container

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
  • TestAccContainerCluster_withMonitoringConfig

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerCluster_withMonitoringConfig[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$

View the build log or the debug log for each test

Copy link

@zli82016 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

@zli82016
Copy link
Member

zli82016 commented Jul 30, 2024

LGTM.

@c2thorn , does merging this PR need to wait for the changes in the main branch merged to 6.0 feature branch first? Thanks.

@c2thorn
Copy link
Member

c2thorn commented Jul 30, 2024

LGTM.

@c2thorn , does merging this PR need to wait for the changes in the main branch merged to 6.0 feature branch first? Thanks.

ideally, yes as it may lead to a merge conflict. Apologies on the delay of the sync, I'll get it started today.

@zli82016
Copy link
Member

ideally, yes as it may lead to a merge conflict. Apologies on the delay of the sync, I'll get it started today.

Thanks, just want to confirm. Sure, take your time.

Copy link

github-actions bot commented Aug 1, 2024

@GoogleCloudPlatform/terraform-team @zli82016 This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

Copy link
Member

@zli82016 zli82016 left a comment

Choose a reason for hiding this comment

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

LGTM

@c2thorn
Copy link
Member

c2thorn commented Aug 5, 2024

The sync was merged, looks like there are merge conflicts now. Apologies and thanks for the wait.

relay_mode field from advanced_datapath_observability_config is superseded
by enable_relay field and will be deprecated on the provider side
in near future.

This is a breaking change according to TF docs and as such, should be
release in the next major release. Deprecation of relay_mode field on
TF side has already happened several months ago, foreshadowing
this change through deprecation message.

Signed-off-by: Michal Siwinski <[email protected]>
@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, 40 insertions(+), 174 deletions(-))
google-beta provider: Diff ( 4 files changed, 40 insertions(+), 174 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

  • Field monitoring_config.advanced_datapath_observability_config.enable_relay changed from optional to required on google_container_cluster - reference
  • Field monitoring_config.advanced_datapath_observability_config.enable_relay default value changed from false to on google_container_cluster - reference
  • Field monitoring_config.advanced_datapath_observability_config.relay_mode within resource google_container_cluster was either removed or renamed - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 200
Passed tests: 190
Skipped tests: 10
Affected tests: 0

Click here to see the affected service packages
  • container

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log

@siwiutki
Copy link
Contributor Author

siwiutki commented Aug 9, 2024

Resolved merge conflicts, seems like it's mergable now.

Copy link
Member

@zli82016 zli82016 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
override-breaking-change Allows a potential breaking change to be merged service/container
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove deprecated field relay_mode from resource google_container_cluster
4 participants