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

fix: update pb files after protobuf was upgraded #38

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

Conversation

mchtech
Copy link
Contributor

@mchtech mchtech commented Jan 3, 2025

fix: #37

ff598bf upgraded protobuf, so pb files should be regenerated.

Signed-off-by: mchtech <michu_an@126.com>
@@ -18,6 +18,7 @@ ADD ./requirements.txt ./
RUN pip install -r ./requirements.txt

ADD ./ ./
RUN python3 /usr/local/lib/python3.11/site-packages/grpc_tools/protoc.py --proto_path=protos/ protos/csi.proto --grpc_python_out=csi/ --python_out=csi/
Copy link
Member

Choose a reason for hiding this comment

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

I think it should not be there

When we change pb files we have to do this and push changes
(We need some CI Tests for checking of that)
also python version is hard-coded

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean we should generate the pb file outside, and COPY it inside?

Copy link
Member

Choose a reason for hiding this comment

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

Yes,
Also it is pointing to site-packages directly, it's better to use python -m instead
I think I will refactor Dockerfile to use Docker multi stage more efficiently

Copy link
Member

Choose a reason for hiding this comment

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

Or we have to separate the stage for the generation

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we also use python -m on mayastor, but it's only for testing there, since the running code is in rust

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but here it is not for test, and it is used for production Dockerfile

@tiagolobocastro tiagolobocastro self-requested a review February 28, 2025 16:01
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.

csi-driver crashed for old pb files
3 participants