-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 FirebaseApp sweepers #7279
Fix FirebaseApp sweepers #7279
Conversation
Waiting to make sure the generated diffs look correct. Will also try running a sweeper locally. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 3 files changed, 11 insertions(+), 11 deletions(-)) |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 3 files changed, 11 insertions(+), 11 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFrameworkProviderMeta_setModuleName|TestAccDataSourceGoogleFirebaseAppleApp|TestAccDataSourceGoogleFirebaseAppleAppConfig|TestAccDataSourceGoogleFirebaseAndroidApp|TestAccFirebaseHostingSite_firebasehostingSiteFullExample|TestAccFirebaseAppleApp_update|TestAccFirebaseAppleApp_firebaseAppleAppFullExample|TestAccFirebaseAppleApp_firebaseAppleAppBasicExample|TestAccFirebaseAndroidApp_update|TestAccFirebaseAndroidApp_firebaseAndroidAppBasicExample|TestAccFirebaseWebApp_firebaseWebAppSkipDelete|TestAccFirebaseHostingSite_firebasehostingSiteUpdate|TestAccFirebaseWebApp_firebaseWebAppFull|TestAccFirebaseWebApp_firebaseWebAppBasicExample|TestAccDataSourceDnsRecordSet_basic|TestAccDataSourceDnsManagedZone_basic|TestAccApigeeAddonsConfig_apigeeAddonsTestExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
/gcbrun |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 3 files changed, 11 insertions(+), 11 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFrameworkProviderMeta_setModuleName|TestAccDataSourceGoogleFirebaseAppleApp|TestAccDataSourceGoogleFirebaseAppleAppConfig|TestAccDataSourceGoogleFirebaseAndroidApp|TestAccFirebaseAppleApp_firebaseAppleAppBasicExample|TestAccFirebaseWebApp_firebaseWebAppSkipDelete|TestAccFirebaseHostingSite_firebasehostingSiteUpdate|TestAccFirebaseWebApp_firebaseWebAppFull|TestAccFirebaseHostingSite_firebasehostingSiteFullExample|TestAccFirebaseAppleApp_update|TestAccFirebaseAppleApp_firebaseAppleAppFullExample|TestAccFirebaseAndroidApp_update|TestAccFirebaseAndroidApp_firebaseAndroidAppBasicExample|TestAccDataSourceDnsManagedZone_basic|TestAccComputeForwardingRule_update|TestAccDataSourceDnsRecordSet_basic |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, adding Stephen for a second review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
5a1d814
to
448a78f
Compare
I accidentally rebased main so had to force-push, but the only change is this: 448a78f The delete URLs in the sweeper were also incorrect, so this change fixes them. Confirmed that the AppleApp sweeper correctly sweeps now, and the |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 3 files changed, 20 insertions(+), 35 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccApigeeAddonsConfig_apigeeAddonsTestExample|TestAccFrameworkProviderMeta_setModuleName|TestAccDataSourceDnsRecordSet_basic|TestAccDataSourceDnsManagedZone_basic |
@roaks3 is there some way we can verify in CI that these are working properly now or do we need to test manually locally? |
@melinath With our current setup, I don't think there is a way to automatically test these before merge. If we decided to build something to support it, you could potentially create fake resources with eg. gcloud, or make a test fail before cleanup, and then run the sweeper and verify the resource no longer exists. That's just one example, but basically, I think there are some options but they would probably introduce some complexity and a longer CI runtime. |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Oh and I guess to add to that, these sweepers in particular were not sweeping, but they did not produce an error or anything that might be easier to detect. Since they were looking in the wrong field of the response, they simple behaved as if there were no resources to sweep. |
* Fix firebase app sweepers * Fix test override quotes * Fix delete urls in sweeper
* Fix firebase app sweepers * Fix test override quotes * Fix delete urls in sweeper
These sweepers have been reporting that there are no sweepable resources, but there are resources accumulating and they have hit the limit. This makes a few changes to get the sweeper working as intended:
(One of the API specs for reference https://firebase.google.com/docs/reference/firebase-management/rest/v1beta1/projects.iosApps)
apps
remove
request to use POSTdisplayName
instead ofname
, because name is server-generateddisplayName
s used for FirebaseApp tests to include sweepable prefixesIf this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)