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

Fix import formats of Firebase apps #7959

Merged
merged 1 commit into from
May 17, 2023

Conversation

rainshen49
Copy link
Contributor

@rainshen49 rainshen49 commented May 16, 2023

  • Use snake_case app_id
  • Addresses an old issue with google_firebase_webapp import

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

Release Note Template for Downstream PRs (will be copied)

firebase: added additional import formats for app resources, removed the need to supply duplicate projects in some cases

@modular-magician
Copy link
Collaborator

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @rileykarson, a repository maintainer, has been assigned to assist you and help review your changes.

❓ First time contributing? Click here for more details

Your assigned reviewer will help review your code by:

  • Ensuring it's backwards compatible, covers common error cases, etc.
  • Summarizing the change into a user-facing changelog note.
  • Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.


Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Have you confirmed these formats locally?

IIUC these all support {{project}} {{name}} and {{name}}, they're using https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/templates/terraform/custom_import/self_link_as_name.erb

{{name}} is technically projects/{{project}}/webApps/{{app_id}} (or the same form for the others), but the others won't work I think.

@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 ( 3 files changed, 12 insertions(+), 11 deletions(-))
Terraform Beta: Diff ( 3 files changed, 12 insertions(+), 11 deletions(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

@ScottSuarez
Copy link
Contributor

/gcbrun

@rainshen49
Copy link
Contributor Author

I just ran through all the formats locally and you're right: only the {{project}} {{name}} and {{name}} one works. All the other formats are wrong. In order for this to work, it looks like I'll need to change the self_link from {{name}} to the full projects/{{project}}/*Apps/{{app_id}} so that app_id is included in the url.

@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 ( 3 files changed, 12 insertions(+), 11 deletions(-))
Terraform Beta: Diff ( 3 files changed, 12 insertions(+), 11 deletions(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

@rainshen49
Copy link
Contributor Author

Fixed & tried out all formats locally. It looks like self_link_as_name.erb is no longer necessary this way.

@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 ( 3 files changed, 18 insertions(+), 14 deletions(-))
Terraform Beta: Diff ( 9 files changed, 99 insertions(+), 41 deletions(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Yep, by moving away from {{name}} we drop that custom code. Plus, we now handle deletion_policy's default value slightly better.

edit: gonna wait for the VCR results, but don't expect any surprises.

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2741
Passed tests 2451
Skipped tests: 283
Affected tests: 7

Action taken

Found 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccComputeFirewallPolicyRule_multipleRules|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccAlloydbCluster_missingLocation|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccAlloydbBackup_missingLocation|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccDataSourceAlloydbLocations_basic

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

Tests passed during RECORDING mode:
TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample[Debug log]
TestAccAlloydbCluster_missingLocation[Debug log]
TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example[Debug log]
TestAccAlloydbBackup_missingLocation[Debug log]
TestAccDataSourceGoogleFirebaseAndroidAppConfig[Debug log]
TestAccDataSourceAlloydbLocations_basic[Debug log]

Tests failed during RECORDING mode:
TestAccComputeFirewallPolicyRule_multipleRules[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

@rileykarson rileykarson merged commit 374cd63 into GoogleCloudPlatform:main May 17, 2023
@rainshen49 rainshen49 deleted the appid branch May 17, 2023 22:03
shourya116 pushed a commit to shourya116/magic-modules that referenced this pull request May 25, 2023
ericayyliu pushed a commit to ericayyliu/magic-modules that referenced this pull request Jul 26, 2023
wj-chen pushed a commit to wj-chen/magic-modules that referenced this pull request Aug 1, 2023
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