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

feat(ci): add missing feat label checker #6451

Merged
merged 13 commits into from
Jul 26, 2023

Conversation

vishwarajanand
Copy link
Contributor

Addresses #6397

@vishwarajanand vishwarajanand marked this pull request as ready for review July 14, 2023 13:37
@vishwarajanand vishwarajanand requested review from a team as code owners July 14, 2023 13:37
.github/workflows/incorrect-pr-label-check.yaml Outdated Show resolved Hide resolved
.github/workflows/incorrect-pr-label-check.yaml Outdated Show resolved Hide resolved
.github/workflows/incorrect-pr-label-check.yaml Outdated Show resolved Hide resolved
.github/workflows/incorrect-pr-label-check.yaml Outdated Show resolved Hide resolved
vishwarajanand and others added 4 commits July 16, 2023 13:37
Co-authored-by: Brent Shaffer <betterbrent@google.com>
Co-authored-by: Brent Shaffer <betterbrent@google.com>
Co-authored-by: Brent Shaffer <betterbrent@google.com>
@vishwarajanand vishwarajanand added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 19, 2023
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

Do we have any PRs where we've tested this?

This can be done by opening a new PR from a branch forked from this one, labeled feat instead of chore but with a new class or method added. This check should fail.

.github/workflows/incorrect-conventional-commit-check.yaml Outdated Show resolved Hide resolved
@vishwarajanand
Copy link
Contributor Author

Do we have any PRs where we've tested this?

Actually, no. Because we need an owlbot PR to test this, so I could not raise one.

@bshaffer
Copy link
Contributor

we need an owlbot PR to test this

You can just comment out this line in the test PR and then you can test it manually.

@vishwarajanand
Copy link
Contributor Author

vishwarajanand commented Jul 24, 2023

Hi @bshaffer, thanks for the tip.

I performed the tests and wrote the results here: #6481 (comment)

Following up on the tests, I made following changes to the PR:

  1. Added types because its supposed to re-run & pass (before reviewers can merge affected PRs) when PR title is changed to feat
  2. Ran the BC compat checker from github.event.pull_request.head.sha to steps.latest-release.outputs.release, because it more accurately represents the latest changes included in the PR
  3. startsWith will change to !startsWith
  4. Removed continue-on-error

One thing to note is that, when a new function is added the failure message says function is removed instead of added:
Error: Method Google\Cloud\Firestore\FirestoreClient#myCustomFunction() was removed

@bshaffer
Copy link
Contributor

when a new function is added the failure message says function is removed instead of added

Yes, I think we may want to do some bash foo here to catch the exception message and throw a different one. And we can get rid of the PR annotations (by removing --format=github-actions from the command) because we don't need them for this - we just want a pass/fail

@vishwarajanand
Copy link
Contributor Author

@bshaffer I added just a single print statement and pass/fail for that, PTAL
A failure looks like this in PR#6481

@bshaffer bshaffer changed the title chore: add missing feat label checker feat(ci): add missing feat label checker Jul 26, 2023
@bshaffer bshaffer enabled auto-merge (squash) July 26, 2023 17:17
@bshaffer bshaffer merged commit 066cc0a into googleapis:main Jul 26, 2023
21 checks passed
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.

2 participants