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.

Sign up for GitHub

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

Jump to bottom

chore: delete deprecated proto top-level folder #729

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: delete deprecated proto top-level folder #729

wants to merge 1 commit into from

Conversation

patrickjcasey
Copy link
Contributor

All .proto files are now stored in hipcheck-common/proto/hipcheck/v1

@patrickjcasey patrickjcasey marked this pull request as ready for review December 17, 2024 15:35
@j-lanson
Copy link
Collaborator

I think Andrew wants the more broken down version of the proto dir going forward. Could you instead replace hipcheck-common/proto with proto and make sure everything compiles / runs properly?

j-lanson
j-lanson requested changes Dec 17, 2024
View reviewed changes
Copy link
Collaborator

@j-lanson j-lanson left a comment

Choose a reason for hiding this comment

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

See above comment

@alilleybrinker
Copy link
Collaborator

We also need to validate this doesn’t break release compilation for anything. One of the challenges with the proto definitions is that they need to be inside the directory of anything released on crates.io. I recommend doing dry runs of cargo publish on all library crates we have to make sure they don’t break. The solution for it is to symlink into the directory so there’s one copy but its replicated into the right places.

@patrickjcasey patrickjcasey force-pushed the patrickjcasey/delete-proto-folder branch from 8941af6 to d89f9b0 Compare December 17, 2024 17:02
@patrickjcasey
Copy link
Contributor Author

I ran the following and they were all successful:

  • cargo publish --dry-run -p hipcheck-sdk
  • cargo publish --dry-run -p hipcheck-macros
  • cargo publish --dry-run -p hipcheck-sdk-macros

@patrickjcasey
Copy link
Contributor Author

When I moved the .proto file, there were some issues with running cargo xtask buf, so I fixed the issues and also added it to CI to ensure buf lint is working as expected

@patrickjcasey patrickjcasey force-pushed the patrickjcasey/delete-proto-folder branch from d89f9b0 to b9f7c01 Compare December 17, 2024 17:10
@j-lanson
Copy link
Collaborator

j-lanson commented Dec 17, 2024 •
edited
Loading

I wanted us to keep the version of proto with the messages subdir, not the monolithic one. Sorry if there was ambiguity.

If possible, I want to see hipcheck-common/proto/messages/*.proto. If other lib crates fail with cargo publish because the proto dir is inside hipcheck-common, then move proto up to the project root. But in either case I want the version that has the messages subdir

@j-lanson j-lanson self-requested a review December 17, 2024 17:15
j-lanson
j-lanson requested changes Dec 17, 2024
View reviewed changes
Copy link
Collaborator

@j-lanson j-lanson left a comment

Choose a reason for hiding this comment

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

See above comment

@patrickjcasey patrickjcasey added the status: needs-rfd This topic is big enough to require an RFD. label Dec 17, 2024
@patrickjcasey
chore: deleted top-level proto directory
bc81e94 
- all .proto files are now stored in hipcheck-common/proto
- fixed multiple buf lints
- re-enabled buf lint in CI

Signed-off-by: Patrick Casey <[email protected]>
@patrickjcasey patrickjcasey force-pushed the patrickjcasey/delete-proto-folder branch from b9f7c01 to bc81e94 Compare December 17, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j-lanson j-lanson j-lanson requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@j-lanson j-lanson

Labels
status: needs-rfd This topic is big enough to require an RFD.
Projects
Hipcheck Dev Roadmap
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@patrickjcasey @j-lanson @alilleybrinker