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 google_container_node_pool #8976

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

shuyama1
Copy link
Member

fix a bug introduced in pr 8884

Release Note Template for Downstream PRs (will be copied)


@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(+), 1 deletion(-))
Terraform Beta: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

@shuyama1
Copy link
Member Author

shuyama1 commented Sep 15, 2023

breaking-chnage and missing-test detectors error seems related to the bug in the base branch now.
Both tpg and tpgb tests passed. Should be safe to merge.

@shuyama1 shuyama1 merged commit 0b07b40 into GoogleCloudPlatform:main Sep 15, 2023
8 checks passed
@shuyama1 shuyama1 deleted the fix_pr_8884 branch September 15, 2023 22:02
@rileykarson
Copy link
Member

rileykarson commented Sep 15, 2023

@trodge @ScottSuarez are we able to identify cases where the base is broken and post a specific message?

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3060
Passed tests 2761
Skipped tests: 298
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
TestAccDataSourceGoogleServiceAccountIdToken_impersonation

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataSourceGoogleServiceAccountIdToken_impersonation[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

@ScottSuarez
Copy link
Contributor

@trodge @ScottSuarez are we able to identify cases where the base is broken and post a specific message?

It should be possible, but could you expand on what UX you're looking for ?

@rileykarson
Copy link
Member

See #8976 (comment) it failed because the base (main) was broken and not head (the PR branch).

The error message "This is usually due to the downstream provider(s) failing to compile." is not very helpful.

@ScottSuarez
Copy link
Contributor

ScottSuarez commented Sep 19, 2023

See #8976 (comment) it failed because the base (main) was broken and not head (the PR branch).

The error message "This is usually due to the downstream provider(s) failing to compile." is not very helpful.

I think that is the wrong place to propose action. The breaking change tool is failing to execute. This is often due to users breaking tpg or tpgb. This case is an outlier and unit test checks already fail in tandem here.

We can certainly add a one off check in the differ that posts if the provider in the base branch fails to compile with a call to action to update the PR branch. Is that the ask?

@rileykarson
Copy link
Member

I think that is the wrong place to propose action. The breaking change tool is failing to execute. This is often due to users breaking tpg or tpgb.

Right, but we can trivially tell here that it isn't failing because the user broke tpg or tpgb, and we can tell because the base failed. If a user had submitted a PR during this window they'd receive the same message even if their change didn't cause the problem, wasting their time as they investigate.

This case is an outlier and unit test checks already fail in tandem here.

Unit tests passed because they run against head, base is what failed:

image

We can certainly add a one off check in the differ that posts if the provider in the base branch fails to compile with a call to action to update the PR branch.

Yes, although probably indicating the contributor should wait until fixed since the average contributor won't be responsible for fixing main, i.e.: "The base branch failed to build successfully, and some checks were unable to run. Please run them again when the base branch is updated."

@ScottSuarez
Copy link
Contributor

ScottSuarez commented Sep 19, 2023

Unit tests passed because they run against head, base is what failed.

Ah yeah this makes sense.

The base branch failed to build successfully, and some checks were unable to run. Please run them again when the base branch is updated

This sounds reasonable. Could you make a ticket?

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