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

multi: Modify buf.yaml for Buf lint and Format with Buf CLI Comamnd("buf format -w") #354

Merged
merged 4 commits into from
Feb 13, 2025

Conversation

shelter2759
Copy link
Contributor

@shelter2759 shelter2759 commented Feb 11, 2025

Issue number: none but related #343


What is the current behavior?

  • Buf lint configuration in buf.yaml does not exist.
  • proto file is not formatted with Buf CLI Command(buf format -w)

What is the new behavior?

  • Buf lint settings in buf.yaml(lint settings are set to ignore errors in the current source code.)
  • Execute “buf format -w” and update the proto file (no change in content)
  • small changes in Dockerfile
  • Add Github Action workflow to execute buf cli (lint, format, breaking)

Does this introduce a breaking change?

  • No

Other information

@shelter2759 shelter2759 changed the title multi: Modify buf.yaml for Buf lint and formatting with buf format -w multi: Modify buf.yaml for Buf lint and Format with buf format -w Feb 11, 2025
@shelter2759 shelter2759 changed the title multi: Modify buf.yaml for Buf lint and Format with buf format -w multi: Modify buf.yaml for Buf lint and Format with Buf CLI Comamnd("buf format -w") Feb 11, 2025
@shelter2759 shelter2759 marked this pull request as ready for review February 11, 2025 13:54
@YusukeShimizu YusukeShimizu self-requested a review February 11, 2025 20:44
@YusukeShimizu
Copy link
Contributor

Thank you for continuing to provide changes aimed at improving the development experience.
How about updating so that developers can also run buf format -w, buf lint, and buf breaking, and furthermore adding these steps to GitHub Actions so that any problematic changes are blocked by CI checks?
I acknowledge the changes of buf.yaml.

@shelter2759
Copy link
Contributor Author

@YusukeShimizu
Appreciate the confirmation and comments.
I will try to support GitHub Actions.

@shelter2759 shelter2759 force-pushed the use-buf-lint branch 4 times, most recently from ef0b165 to 268c6b9 Compare February 12, 2025 15:20
@shelter2759
Copy link
Contributor Author

@YusukeShimizu
I added job to run buf cli(buf-action) in github action workflow.(268c6b9)

Please check when you have time.
Thank you.

@YusukeShimizu
Copy link
Contributor

YusukeShimizu commented Feb 12, 2025

I think introducing buf-action is appropriate, and there’s no issue with the CI content.

[Q]It’s not entirely clear how developers are expected to run Buf commands (e.g., buf format -w) locally. We can generate code with “make all-rpc-docker,” but what should we do for other commands?
If you’re in the dev container, Buf is installed using “postCreateCommand,” but what happens in other cases? If there isn’t a provided method, I’d like to document that you either use Docker (like “make all-rpc-docker”) or individually install Buf yourself.

@YusukeShimizu
Copy link
Contributor

It seems that all-rpc is also a command that requires prior environment setup.
In line with the above considerations, it may be necessary to either update or remove it

@shelter2759
Copy link
Contributor Author

shelter2759 commented Feb 13, 2025

Thank you for raising this question.

I recommend to install Buf Locally, developers can install Buf on their local machine following the official Buf installation guide and run the required commands directly.

Where would be the appropriate place to include the documentation, if necessary? Would it be acceptable to add it to the README?

I updated README and add usage_buf.md, please check.

I have added other commands, such as lint and format, to the Makefile, just like all-rpc.
c41c475

Copy link
Contributor

@YusukeShimizu YusukeShimizu left a comment

Choose a reason for hiding this comment

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

LGTM.
We might consider installing buf via nix flake, but it’s already quite user-friendly as it is. Thanks!

Since this change is intended to enhance the developer experience without affecting behavior, I’ll merge it.

@YusukeShimizu YusukeShimizu merged commit 10abc45 into ElementsProject:master Feb 13, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants