Skip to content

vet: add dependency checks#7766

Merged
dfawley merged 2 commits intogrpc:masterfrom
dfawley:deps
Oct 25, 2024
Merged

vet: add dependency checks#7766
dfawley merged 2 commits intogrpc:masterfrom
dfawley:deps

Conversation

@dfawley
Copy link
Copy Markdown
Member

@dfawley dfawley commented Oct 21, 2024 •
edited
Loading

Fixes #7690

This change adds a new GA workflow to compare dependencies before and after a PR. If they change, it will report a red X. We will not require this check to pass for merging, but it should be inspected to see if the change in dependencies is reasonable and expected.

RELEASE NOTES: none

@dfawley dfawley added the Area: Testing Includes tests and testing utilities that we have for unit and e2e tests within our repo. label Oct 21, 2024
@dfawley dfawley requested a review from easwars October 21, 2024 22:35
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 21, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.24%. Comparing base (4bb0170) to head (35ebde4).
Report is 6 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7766      +/-   ##
==========================================
+ Coverage   80.25%   81.24%   +0.99%     
==========================================
  Files         367      368       +1     
  Lines       36622    36779     +157     
==========================================
+ Hits        29391    29882     +491     
+ Misses       6043     5653     -390     
- Partials     1188     1244      +56

see 26 files with indirect coverage changes

easwars
easwars reviewed Oct 21, 2024
View reviewed changes
Comment thread scripts/vet.sh Outdated
Comment thread scripts/vet.sh Outdated
@easwars easwars assigned dfawley and unassigned easwars Oct 21, 2024
@dfawley dfawley assigned easwars and unassigned dfawley Oct 21, 2024
@dfawley dfawley added this to the 1.69 Release milestone Oct 21, 2024
@dfawley dfawley added Type: Testing and removed Area: Testing Includes tests and testing utilities that we have for unit and e2e tests within our repo. labels Oct 21, 2024
easwars
easwars approved these changes Oct 22, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@easwars easwars left a 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 the approach here.

Maybe:

  • we just need to make enough noise for code authors and reviewers to be mindful of changes to dependencies
  • put the instructions to regenerate deps somewhere accessible (for new folks on the team and for external contributors so that reviewers can point to it easily)?

Comment thread scripts/vet.sh Outdated
@easwars easwars assigned dfawley and unassigned easwars Oct 22, 2024
@dfawley
Copy link
Copy Markdown
Member Author

dfawley commented Oct 23, 2024

put the instructions to regenerate deps somewhere accessible

I added an error message in vet and also made a separate script for generating. PTAL!

easwars
easwars previously approved these changes Oct 23, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes.

Comment thread scripts/vet.sh Outdated
@dfawley dfawley force-pushed the deps branch 13 times, most recently from 5eab4c2 to ac76cff Compare October 23, 2024 21:21
@dfawley dfawley force-pushed the deps branch 6 times, most recently from fe6f52f to 6f9ac3d Compare October 23, 2024 22:35
@dfawley
Copy link
Copy Markdown
Member Author

dfawley commented Oct 23, 2024

Added a new workflow for this instead that turns red when deps change in any way. Here's what it looks like when there is a failure:

https://github.com/grpc/grpc-go/actions/runs/11489199182/job/31977501207?pr=7766#step:4:19

And here's a pass from this PR:

https://github.com/grpc/grpc-go/actions/runs/11489222235/job/31977569900?pr=7766#step:4:19

I had to temporarily copy the gen-deps.sh script into /tmp in the workflow, because upstream/master doesn't have it yet. So I could do this as two changes, one that adds the script and one that adds the workflow later, or just delete the indicated lines before merging. Let me know if you prefer the former.

@dfawley dfawley assigned easwars and unassigned dfawley Oct 23, 2024
@dfawley dfawley requested a review from easwars October 23, 2024 22:45
easwars
easwars reviewed Oct 24, 2024
View reviewed changes
Comment thread .github/workflows/deps.yml
Comment thread .github/workflows/deps.yml Outdated
@easwars easwars assigned dfawley and unassigned easwars Oct 24, 2024
@easwars
Copy link
Copy Markdown
Contributor

easwars commented Oct 24, 2024

We also need a new PR description since we have a new approach now.

@dfawley dfawley assigned easwars and unassigned dfawley Oct 25, 2024
@easwars easwars assigned dfawley and unassigned easwars Oct 25, 2024
@dfawley dfawley merged commit 94e1b29 into grpc:master Oct 25, 2024
1 check passed
@dfawley dfawley deleted the deps branch October 25, 2024 20:42
@dfawley dfawley mentioned this pull request Oct 29, 2024
Merged
misvivek pushed a commit to misvivek/grpc-go that referenced this pull request Nov 7, 2024
@dfawley @misvivek
vet: add dependency checks (grpc#7766)
52eee99
@arjan-bal arjan-bal mentioned this pull request Feb 17, 2025
Closed
@dashpole dashpole mentioned this pull request Feb 19, 2025
Merged
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@easwars easwars easwars approved these changes

Assignees

@dfawley dfawley

Labels

Type: Testing

Projects

None yet

Milestone

1.69 Release

Development

Successfully merging this pull request may close these issues.

Add a test for dependencies of grpc package

2 participants

@dfawley @easwars