-
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
VCR split #9392
VCR split #9392
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
1 similar comment
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
1 similar comment
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Tests analyticsTotal tests: Click here to see the affected service packages
|
Added Already cleaned up the cassettes recorded for testing this PR change. |
for file in $gofiles | ||
do | ||
if [[ $file = google-beta/services* ]]; then | ||
affected_services[$(echo "$file" | awk -F / '{ print $3 }')]=1 |
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.
Could you add a comment explaining what this awk command is doing?
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.
+1
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.
Added. Let me know if any part is still unclear and needs more comments. Thanks:)
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 overall
|
||
TESTS_TERMINATED=$(grep "^cannot run Terraform provider tests" replaying_test.log) | ||
|
||
counter=1 |
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.
it looks like we're losing this counter
functionality - why is that safe / desirable to do as part of this change? They seem unrelated.
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.
When VCR was initially set up in GCB, we used to run into replaying tests got terminated due to non-provider-related issues, so we rerun the tests if that error occurred (with max retry: 3) example: #5881 (comment)
The issue should be resolved after we pre-install Terraform, but we never removed the logic. I've never seen any VCR test rerun needed for replaying mode after #6034, so I think it should be safe to remove it.
I agree it's unrelated - I could keep it for now and remove it in a separate 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.
I'm fine with keeping this in
for file in $gofiles | ||
do | ||
if [[ $file = google-beta/services* ]]; then | ||
affected_services[$(echo "$file" | awk -F / '{ print $3 }')]=1 |
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.
+1
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
* split VCR tests * tests * fix * fix checker * more tests * fix test code * add magician comment * fix typo * update service packages detector * revert test changes * added more comments
* split VCR tests * tests * fix * fix checker * more tests * fix test code * add magician comment * fix typo * update service packages detector * revert test changes * added more comments
* split VCR tests * tests * fix * fix checker * more tests * fix test code * add magician comment * fix typo * update service packages detector * revert test changes * added more comments
Release Note Template for Downstream PRs (will be copied)