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

Add CI actions #11

Merged
merged 15 commits into from
Dec 6, 2023
Merged

Add CI actions #11

merged 15 commits into from
Dec 6, 2023

Conversation

XuanWang-Amos
Copy link
Collaborator

Changes in this PR

  • Add unittest, black, isort and pylint check to GitHub actions.
  • Added proto filed used for testing to this repo.

@XuanWang-Amos XuanWang-Amos marked this pull request as ready for review November 18, 2023 00:26
.pylintrc Outdated Show resolved Hide resolved
.github/workflows/psm-interop.yaml Show resolved Hide resolved
.github/workflows/psm-interop.yaml Outdated Show resolved Hide resolved
.github/workflows/psm-interop.yaml Show resolved Hide resolved
.github/workflows/psm-interop.yaml Outdated Show resolved Hide resolved
run: >
python -m grpc_tools.protoc --proto_path=.
--python_out=. --grpc_python_out=.
protos/empty.proto
Copy link
Member

Choose a reason for hiding this comment

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

We probably should keep the packages path after protos/: protos/grpc/testing.
@gnossen - curious what you'd recommend here. Does the path really matter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's the benefits of keeping it? I thought those are just paths to the .proto files.

Copy link
Member

@sergiitk sergiitk Nov 21, 2023

Choose a reason for hiding this comment

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

They do declare package grpc.testing;, and IRRC file path should correspond to the packages.
In practice, this means that the imports will look different:

from protos.grpc.testing import messages_pb2

instead of

from protos import messages_pb2

Then if we need to embed other protos (f.e. xds), they'd be in their own package too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checked with Richard offline, it's better to move them to correct location.

@sergiitk
Copy link
Member

Thank you so much, @XuanWang-Amos, this looks great! Just left some minor notes.

.github/workflows/psm-interop.yaml Show resolved Hide resolved
bin/isort.sh Outdated Show resolved Hide resolved
bin/black.sh Outdated Show resolved Hide resolved
Copy link
Member

@sergiitk sergiitk left a comment

Choose a reason for hiding this comment

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

Thank you so much!

@XuanWang-Amos XuanWang-Amos merged commit 02a17f3 into main Dec 6, 2023
6 checks passed
@XuanWang-Amos XuanWang-Amos deleted the revert-10-remove_unreviewed_changes branch December 6, 2023 20:01
sergiitk added a commit that referenced this pull request Dec 8, 2023
* Revert "Remove unreviewed changes (#10)"

This reverts commit 7b0e3f1.

* Changes for this repo

* Fix indent issue

* Try not install Python for pylint

* Remove strategy

* Remvoe name

* Changes from comments

* Try fix path issue

* Move .pylintrc-tests back to root

* Use two steps for pylint

* try pylint in another folder

* Try rename black.toml to pyproject.toml

* Move protos

* Fix proto path

* Changes based on comments

---------

Co-authored-by: Sergii Tkachenko <[email protected]>
sergiitk added a commit that referenced this pull request Dec 8, 2023
* Revert "Remove unreviewed changes (#10)"

This reverts commit 7b0e3f1.

* Changes for this repo

* Fix indent issue

* Try not install Python for pylint

* Remove strategy

* Remvoe name

* Changes from comments

* Try fix path issue

* Move .pylintrc-tests back to root

* Use two steps for pylint

* try pylint in another folder

* Try rename black.toml to pyproject.toml

* Move protos

* Fix proto path

* Changes based on comments

---------

Co-authored-by: Sergii Tkachenko <[email protected]>
sergiitk added a commit that referenced this pull request Dec 8, 2023
* Revert "Remove unreviewed changes (#10)"

This reverts commit 7b0e3f1.

* Changes for this repo

* Fix indent issue

* Try not install Python for pylint

* Remove strategy

* Remvoe name

* Changes from comments

* Try fix path issue

* Move .pylintrc-tests back to root

* Use two steps for pylint

* try pylint in another folder

* Try rename black.toml to pyproject.toml

* Move protos

* Fix proto path

* Changes based on comments

---------

Co-authored-by: Sergii Tkachenko <[email protected]>
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