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

Remove protoc compiling from CI #59

Merged
merged 2 commits into from
Sep 26, 2023
Merged

Conversation

dbwiddis
Copy link
Member

Description

Since protobuf autogenerated files change rarely (if ever) and require a few extra steps beyond compiling, it's better to not use them in the build. Instructions for the rare case of needing to recompile are added to the Developer Guide

Issues Resolved

Fixes #23

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Daniel Widdis <widdis@gmail.com>
@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #59 (35c9ecf) into main (7dc19ea) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main       #59   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           55        55           
  Lines         1462      1462           
=========================================
  Hits          1462      1462           

see 2 files with indirect coverage changes

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

How about adding a poetry exec proto (pyproject.toml) for these commands and note in dev guide?

@@ -170,6 +171,15 @@ TOTAL 1207 79 93%

You can run a combination of these by installing [poetry-exec-plugin](https://github.com/keattang/poetry-exec-plugin) once and using the `poetry exec coverage` shortcut.

### Protobuf Class Generation

Some Transport Requests use [Protobuf](https://protobuf.dev/) messages. These offer benefits including better reverse compatibility and easier cross-platform class generation. When using a new or updated proto definition (`.proto` file extension) you will need to compile it for use with python.
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to explain what these are a little better, with maybe a link to where the files come from in OpenSearch core?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice to explain what these are a little better,

The link to protobuf.dev is pretty clear on the messages.

with maybe a link to where the files come from in OpenSearch core?

Good idea, I'll add.

@dbwiddis
Copy link
Member Author

How about adding a poetry exec proto (pyproject.toml) for these commands and note in dev guide?

These are done rarely enough it's not worth automating.

image

Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dblock
Copy link
Member

dblock commented Sep 26, 2023

These are done rarely enough it's not worth automating.

I won't argue. IMHO copy-pasting from developer guide line by line is annoying, and we're going to be reaching to the code that you're deleting here so I don't have to turn my brain on to generate these files in a few months. Especially the | xargs sed -i.bak -E 's/(USE_C_DESCRIPTORS == False:)/\1 # pragma: no cover/g' part that I am 100% sure the contributor will forget after reading "Add # pragma: no cover to the conditional test" which nobody will know the meaning or the reason for ;)

1 similar comment
@dblock
Copy link
Member

dblock commented Sep 26, 2023

These are done rarely enough it's not worth automating.

I won't argue. IMHO copy-pasting from developer guide line by line is annoying, and we're going to be reaching to the code that you're deleting here so I don't have to turn my brain on to generate these files in a few months. Especially the | xargs sed -i.bak -E 's/(USE_C_DESCRIPTORS == False:)/\1 # pragma: no cover/g' part that I am 100% sure the contributor will forget after reading "Add # pragma: no cover to the conditional test" which nobody will know the meaning or the reason for ;)

@dblock dblock merged commit 7656a80 into opensearch-project:main Sep 26, 2023
7 checks passed
@dbwiddis dbwiddis deleted the ci branch September 30, 2023 06:06
Jatin-tec pushed a commit to Jatin-tec/opensearch-sdk-py that referenced this pull request Nov 10, 2023
* Remove protoc compiling from CI

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Link to OpenSearch .proto files

Signed-off-by: Daniel Widdis <widdis@gmail.com>

---------

Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Jatin <jatin.kshatriya2821@gmail.com>
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.

Handling auto-generated protobuf files
2 participants