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 firebase_project_location #8783

Conversation

rainshen49
Copy link
Contributor

@rainshen49 rainshen49 commented Aug 28, 2023

Throw an error when users try to use the google_firebase_project_location resource in the 5.0.0 version.

closes hashicorp/terraform-provider-google#15763

If this PR is for Terraform, I acknowledge that I have:

Release Note Template for Downstream PRs (will be copied)

firebase: removed `google_firebase_project_location`

@modular-magician
Copy link
Collaborator

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will run automatically.

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

Terraform GA: Diff ( 1 file changed, 1 insertion(+), 30 deletions(-))
Terraform Beta: Diff ( 3 files changed, 4 insertions(+), 176 deletions(-))

Copy link
Contributor

@roaks3 roaks3 left a comment

Choose a reason for hiding this comment

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

Looks like we're seeing this error in the build:

google-beta/services/firebase/resource_firebase_project_location.go:68:2: config declared but not used

I think it is because the config var used by the create function normally is not being used anymore. Do you know if there is precedence for the pattern you're using here?

mmv1/products/firebase/ProjectLocation.yaml Show resolved Hide resolved
@rainshen49
Copy link
Contributor Author

I don't find a precedent. I'm trying to follow https://developer.hashicorp.com/terraform/plugin/sdkv2/best-practices/deprecations#provider-data-source-or-resource-removal, which says to

remove all code associated with the deprecated data source or resource except for the schema and replace the Create and Read functions to always return an error

Since this resource doesn't have any attributes, I don't think anyone would be reading them, but the Create function should be blocked.

Is there a better way to do that than custom_create?

@modular-magician
Copy link
Collaborator

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

Breaking Change Detection Failed

The breaking change detector crashed during execution. This is usually due to the downstream provider(s) failing to compile. Please investigate or follow up with your reviewer.

Diff report

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

Terraform GA: Diff ( 1 file changed, 1 insertion(+), 30 deletions(-))
Terraform Beta: Diff ( 3 files changed, 191 insertions(+), 329 deletions(-))

@rainshen49 rainshen49 force-pushed the firebase-deprecate-default-location branch from 6f6b862 to 495f23b Compare August 31, 2023 13:41
@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.

Terraform GA: Diff ( 1 file changed, 1 insertion(+), 30 deletions(-))
Terraform Beta: Diff ( 3 files changed, 6 insertions(+), 175 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3001
Passed tests 2699
Skipped tests: 301
Affected tests: 1

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
TestAccDataprocJobIamPolicy

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

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

Rerun these tests in REPLAYING mode to catch issues

$\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
Contributor

@roaks3 roaks3 left a comment

Choose a reason for hiding this comment

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

@c2thorn would you mind weighing in on this? I wasn't able to find any precedent for deprecating resources, but this approach makes sense to me and seems to match the guidance from HashiCorp.

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

Terraform GA: Diff ( 1 file changed, 1 insertion(+), 30 deletions(-))
Terraform Beta: Diff ( 3 files changed, 6 insertions(+), 175 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3001
Passed tests 2700
Skipped tests: 301
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

@rainshen49
Copy link
Contributor Author

Friendly ping @c2thorn

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.

For other removed resources (like google_project_services and game services) we just completely delete the related code and do not leave stubbed resources. It would be nice for users to get a specific message stating that the resource is no longer available, but as of now we prefer to just completely remove.

@modular-magician
Copy link
Collaborator

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

Breaking Change(s) Detected

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

  • Resource google_firebase_project_location 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.

Diff report

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

Terraform GA: Diff ( 1 file changed, 115 deletions(-))
Terraform Beta: Diff ( 4 files changed, 2 insertions(+), 473 deletions(-))
TF Conversion: Diff ( 1 file changed, 69 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2991
Passed tests 2690
Skipped tests: 301
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

@c2thorn
Copy link
Member

c2thorn commented Sep 6, 2023

We should add the actual deprecation message and upgrade guide note in the main branch. Does a PR for that exist already? @rainshen49
Also, mind updating the PR/changelog note to be specific about removing?

@rainshen49
Copy link
Contributor Author

Updated PR description.
Yes, the deprecation message and migration guide PR has been merged #8301

@c2thorn c2thorn changed the title Deprecate firebase_project_location Remove firebase_project_location Sep 7, 2023
@c2thorn
Copy link
Member

c2thorn commented Sep 7, 2023

I've added hashicorp/terraform-provider-google#15763 for our tracking efforts. If there is a buganizer number, do you mind adding the b/##### to the issue? @rainshen49

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

Successfully merging this pull request may close these issues.

4 participants