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

scripts: improve regenerate.sh to use the correct proto compiler version #7064

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

aranjans
Copy link
Collaborator

@aranjans aranjans commented Mar 25, 2024

Fixes #6583

RELEASE NOTES: n/a

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.70%. Comparing base (adf976b) to head (b3106cc).
Report is 46 commits behind head on master.

❗ Current head b3106cc differs from pull request most recent head 5878795. Consider uploading reports for the commit 5878795 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7064      +/-   ##
==========================================
- Coverage   81.24%   80.70%   -0.54%     
==========================================
  Files         345      346       +1     
  Lines       33941    33802     -139     
==========================================
- Hits        27574    27280     -294     
- Misses       5202     5355     +153     
- Partials     1165     1167       +2     

see 33 files with indirect coverage changes

@aranjans aranjans force-pushed the aranjans_6583 branch 2 times, most recently from 8bb8747 to af6bc49 Compare March 25, 2024 17:33
@dfawley dfawley added the Type: Meta Github repo, process, etc label Mar 25, 2024
@dfawley dfawley added this to the 1.64 Release milestone Mar 25, 2024
@dfawley dfawley requested a review from arvindbr8 March 25, 2024 17:37
@aranjans aranjans marked this pull request as ready for review March 25, 2024 17:37
@aranjans aranjans force-pushed the aranjans_6583 branch 2 times, most recently from e404049 to a582a54 Compare March 26, 2024 06:59
Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

I would prefer if we didnt track the version of protoc in different places.

What might be nicer is if both regenerate.sh and vet.sh points to the same place to install protoc. Also note ./vet.sh -install basically does the same thing of installing protoc for Github Actions.

I prefer creating a new script which installs protoc based on the ${OS}. And for ./vet -install I could call into the script to install the linux and x86_64 flavor of protoc. And do something similar for regenerate.sh

vet.sh Outdated Show resolved Hide resolved
regenerate.sh Show resolved Hide resolved
regenerate.sh Outdated Show resolved Hide resolved
@aranjans aranjans force-pushed the aranjans_6583 branch 12 times, most recently from 20c21cb to a880e4d Compare March 27, 2024 10:00
@dfawley dfawley assigned arvindbr8 and unassigned aranjans Mar 27, 2024
Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

I've made a few comments on your change. Could you please take a look at it?

protoc_installer.sh Outdated Show resolved Hide resolved
protoc_installer.sh Outdated Show resolved Hide resolved
protoc_installer.sh Outdated Show resolved Hide resolved
protoc_installer.sh Outdated Show resolved Hide resolved
protoc_installer.sh Outdated Show resolved Hide resolved
regenerate.sh Outdated Show resolved Hide resolved
regenerate.sh Outdated Show resolved Hide resolved
regenerate.sh Outdated Show resolved Hide resolved
regenerate.sh Outdated Show resolved Hide resolved
protoc_installer.sh Outdated Show resolved Hide resolved
@arvindbr8 arvindbr8 assigned aranjans and unassigned arvindbr8 Mar 29, 2024
@aranjans aranjans force-pushed the aranjans_6583 branch 2 times, most recently from 2cddfeb to cc78601 Compare April 1, 2024 06:40
@aranjans aranjans removed their assignment May 2, 2024
Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

A couple of comments from the previous pass still seems to be unaddressed:

  1. https://github.com/grpc/grpc-go/pull/7064/files#r1569226189
  2. https://github.com/grpc/grpc-go/pull/7064/files#r1569224986

I also have some more comments. I think it looks overall okay if the aim is to fix the current issue. But could you please open a new issue which talks about the TODOs from this PR?

scripts/vet-proto.sh Outdated Show resolved Hide resolved
scripts/regenerate.sh Outdated Show resolved Hide resolved
Copy link

github-actions bot commented May 9, 2024

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added the stale label May 9, 2024
@aranjans
Copy link
Collaborator Author

aranjans commented May 9, 2024

fyi: was caught up in other issues. I am working on this right now on priority, will update the PR soon.

@aranjans
Copy link
Collaborator Author

aranjans commented May 9, 2024

@arvindbr8 In contrast to this comment, first comment is resolved but i didn't understand the second one. Can you be more specific on that?

@aranjans aranjans assigned arvindbr8 and unassigned aranjans May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Type: Meta Github repo, process, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve regenerate.sh to use the correct proto compiler version
4 participants