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

chore(internal/kokoro): port check_incompat_changes #3769

Merged
merged 6 commits into from
Mar 16, 2021

Conversation

noahdietz
Copy link
Contributor

@noahdietz noahdietz commented Mar 3, 2021

Ports the apidiff logic of check_incompat_changes.sh into a Go program internal/apidiff.

This new version reads from the internal/.repo-metadata-full.json file and runs apidiff against all packages labeled as ga.

When it encounters a manual client or a generated subclient of a submodule, it attempts to find the submodule, change into that directory, and run apidiff from there.

It will collect all errors/diffs and report them at the end. The script will only fail if there are diffs. If there is an error during apidiff of one of the packages, it will be reported, but it will not fail the script.

Fixes #3764 by parsing the repo-metadata, not a regexp in a bash script.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 3, 2021
@noahdietz noahdietz changed the title internal/kokoro: port check_incompat_changes chore(internal/kokoro): port check_incompat_changes Mar 3, 2021
@noahdietz noahdietz marked this pull request as ready for review March 3, 2021 21:53
@noahdietz noahdietz requested a review from a team as a code owner March 3, 2021 21:53
@noahdietz
Copy link
Contributor Author

Currently failing because apidiff is complaining about a constant that doesn't actually change in pubsub

cloud.google.com/go/pubsub:
- MaxPublishRequestBytes: value changed from 0.000582077 to 10000000

Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

Mostly nits, overall LGTM.

internal/apidiff/apidiff.go Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
@noahdietz noahdietz requested a review from codyoss March 4, 2021 22:34
@@ -21,33 +21,4 @@ if [[ `go version` != *"go1.16"* ]]; then
exit 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codyoss I could move this check into shouldRun() if you want to get rid of this script altogether. WDYT?

@noahdietz
Copy link
Contributor Author

Blocked by golang/go#44796

internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
@noahdietz noahdietz requested review from crwilcox, kolea2, skuruppu, tritone and a team as code owners March 10, 2021 23:37
@noahdietz noahdietz requested a review from a team March 10, 2021 23:37
@noahdietz
Copy link
Contributor Author

Oops, I messed up my git history some how and ive got waaaay too many commits that don't belong. Ignore this please!

@noahdietz noahdietz requested review from tbpg and removed request for a team, crwilcox, skuruppu, tritone and kolea2 March 10, 2021 23:44
@noahdietz
Copy link
Contributor Author

@tbpg @codyoss can I get another pass on this? I am manually ignoring the incorrect pubsub error (if it is the only error there is for that pkg)

Copy link
Contributor

@tbpg tbpg left a comment

Choose a reason for hiding this comment

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

Sounds good to move this to a Go program, but I'm having trouble following the flow of execution. I left some comments about the APIs. Overall, it feels more... complicated (?) than I expected. Hopefully the suggestions I gave help simplify the flow.

internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
@noahdietz
Copy link
Contributor Author

Overall, it feels more... complicated (?) than I expected

Yeah that's reasonable - it is more complicated. This now:

  • supports diffing submodules, which the previous script didn't
  • reads a config file instead of iterating over a hard coded list
  • collects all diffs/errors before reporting instead of exiting at first issue

So it's not a direct port, it's a "port+" :) Your suggestions are totally valid though.

@noahdietz noahdietz requested a review from tbpg March 11, 2021 22:43
Copy link
Contributor

@tbpg tbpg left a comment

Choose a reason for hiding this comment

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

Much more understandable -- thank you!

internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
internal/apidiff/apidiff.go Outdated Show resolved Hide resolved
@noahdietz
Copy link
Contributor Author

@codyoss I think you need to approve since you requested changes.

@codyoss
Copy link
Member

codyoss commented Mar 16, 2021

@noahdietz Sorry about that, did not realize this was waiting on me.

@noahdietz
Copy link
Contributor Author

Only waiting like 1 min :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

internal/kokoro: collect packages to apidiff with a regexp, not a list
3 participants