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: add or vendor proto packages from Go dependencies #3724

Merged
merged 34 commits into from
Nov 14, 2023

Conversation

jeronimoalbi
Copy link
Member

Closes #3635

Third party deps are added to app's Buf config when the dependency is
not present and the Go package protos have a Buf config with a `name`
assigned.
@jeronimoalbi jeronimoalbi added the type:feat To implement new feature. label Nov 1, 2023
@jeronimoalbi jeronimoalbi self-assigned this Nov 1, 2023
@github-actions github-actions bot added component:ci CI/CD workflow and automated jobs. component:configs component:packages labels Nov 1, 2023
@github-actions github-actions bot added component:cmd type:services Service-related issues. labels Nov 2, 2023
@jeronimoalbi jeronimoalbi force-pushed the feat/buf-dependency-improvements branch from ecbec29 to 1c97777 Compare November 2, 2023 17:57
@jeronimoalbi
Copy link
Member Author

jeronimoalbi commented Nov 2, 2023

There are two cases handled by this PR when a Go dependency contains proto files:

  • The dependency contains a Buf config with the optional name property setted
  • The dependency only contains proto files or a Buf config without a name property

For the first case a new dependency entry is added to app's Buf config; For the second case proto files are copied or exported to a proto_vendor directory.

All scaffold commands handle these cases by default.

All generate commands optionally handle these cases when the --enable-proto-vendor flag is present.

@jeronimoalbi
Copy link
Member Author

jeronimoalbi commented Nov 3, 2023

Feature can be manually tested by scaffolding a chain which will have a new Buf dependency buf.build/cosmos/ibc added to proto/buf.yaml during scaffold which is not included in the default CLI app templates.

After that remove the buf.build/cosmos/ibc from proto/buf.yaml and run buf mod prune ./proto to cleanup the dependency. Running ignite generate proto-go --clear-cache -y --enable-proto-vendor should add it back.

Finally remove the buf.build/cosmos/ibc, run buf mod prune ./proto to cleanup the dependency again and temporary remove the name property from $(go env GOMODCACHE)/github.com/cosmos/ibc-go/[email protected]/proto/buf.yaml. Running ignite generate proto-go --clear-cache -y --enable-proto-vendor should vendor the proto files this time.

At this point I'm not sure how to properly test it using CI tests.

@jeronimoalbi jeronimoalbi marked this pull request as ready for review November 3, 2023 14:36
Pantani
Pantani previously approved these changes Nov 9, 2023
@jeronimoalbi
Copy link
Member Author

jeronimoalbi commented Nov 9, 2023

Maybe the --update-buf-module flag could be renamed to --enable-proto-vendor or --enable-proto-vendoring 🤔

If you prefer any of those names over the current one let me know and I can change it.

@Pantani
Copy link
Collaborator

Pantani commented Nov 9, 2023

--enable-proto-vendor

my vote is for --enable-proto-vendor

@Ehsan-saradar
Copy link
Contributor

Feature can be manually tested by scaffolding a chain which will have a new Buf dependency buf.build/cosmos/ibc added to proto/buf.yaml during scaffold which is not included in the default CLI app templates.

Hey @jeronimoalbi I don't remember an option for adding a buf dependency in scaffold chain command. Could you add some details?

@jeronimoalbi
Copy link
Member Author

Hey @jeronimoalbi I don't remember an option for adding a buf dependency in scaffold chain command. Could you add some details?

There is not option in the CLI to explicitly add a Buf dependency, at this point developers should add them by hand by changing the buf.yaml file.
The changes in this PR try to discover Buf dependencies from the app's Go depedencies. The only case where a dependency is added to the buf.yaml by the CLI is when a Go dependency contains proto files and a buf.yaml file with the optional name property specified, in which case the name is added to app's Buf dependencies and then the Buf lock file is updated.

The scaffold commands automatically try to discover proto dependencies that might be missing, while the generate commands should opt-in by using the --enable-proto-vendor.

Copy link
Contributor

@Ehsan-saradar Ehsan-saradar left a comment

Choose a reason for hiding this comment

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

@jeronimoalbi I originally thought that the "during scaffold" part of your comment meant that you added an option to add buf dependencies during chain scaffold. 😅

Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Merging #3724 (327ca6c) into main (57813c9) will decrease coverage by 0.20%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3724      +/-   ##
==========================================
- Coverage   24.83%   24.64%   -0.20%     
==========================================
  Files         295      295              
  Lines       24181    24376     +195     
==========================================
+ Hits         6006     6008       +2     
- Misses      17628    17822     +194     
+ Partials      547      546       -1     
Files Coverage Δ
ignite/services/scaffolder/scaffolder.go 0.00% <0.00%> (ø)
ignite/cmd/generate_typescript_client.go 0.00% <0.00%> (ø)
ignite/cmd/generate_composables.go 0.00% <0.00%> (ø)
ignite/cmd/generate_go.go 0.00% <0.00%> (ø)
ignite/cmd/generate_hooks.go 0.00% <0.00%> (ø)
ignite/cmd/generate_openapi.go 0.00% <0.00%> (ø)
ignite/cmd/generate_pulsar.go 0.00% <0.00%> (ø)
ignite/cmd/generate_vuex.go 0.00% <0.00%> (ø)
ignite/pkg/cosmosgen/generate_go.go 0.00% <0.00%> (ø)
ignite/pkg/cosmosgen/generate_openapi.go 0.00% <0.00%> (ø)
... and 6 more

... and 1 file with indirect coverage changes

@ilgooz ilgooz enabled auto-merge (squash) November 14, 2023 20:30
@ilgooz ilgooz disabled auto-merge November 14, 2023 20:41
@ilgooz ilgooz merged commit 6735957 into main Nov 14, 2023
26 checks passed
@ilgooz ilgooz deleted the feat/buf-dependency-improvements branch November 14, 2023 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:ci CI/CD workflow and automated jobs. component:cmd component:configs component:packages type:feat To implement new feature. type:services Service-related issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: buf improvements
4 participants