Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Update validate module to protoc-gen-validate 0.6.1 #165

Merged
merged 3 commits into from
May 6, 2021
Merged

Update validate module to protoc-gen-validate 0.6.1 #165

merged 3 commits into from
May 6, 2021

Conversation

hanzo
Copy link
Contributor

@hanzo hanzo commented May 6, 2021

TL;DR

The current version of validate/validate_pb2.py was generated from validate.proto in protoc-gen-validate as of version 0.4.1. This PR updates the generated code to match protoc-gen-validate v0.6.1, the latest release. This will prevent collisions with other libraries that are using protoc-gen-validate v0.6.1.

Type

  • Bug Fix
  • Feature
  • Plugin

@hanzo hanzo marked this pull request as ready for review May 6, 2021 00:16
@hanzo hanzo marked this pull request as draft May 6, 2021 00:21
@milton0825
Copy link
Contributor

cc: @kumare3 @EngHabu

@akhurana001 akhurana001 marked this pull request as ready for review May 6, 2021 18:17
@EngHabu
Copy link
Contributor

EngHabu commented May 6, 2021

We generate by running make generate I expect there should be a version change in some file where we take depedency on that tool... I only see the generated code in the PR...

@dkunitsk
Copy link

dkunitsk commented May 6, 2021

+1

@kumare3
Copy link
Contributor

kumare3 commented May 6, 2021

@hanzo how did you generate the code?

@milton0825
Copy link
Contributor

Possibly just copied the generated code from https://github.com/envoyproxy/protoc-gen-validate ?

@hanzo
Copy link
Contributor Author

hanzo commented May 6, 2021

Correct me if I'm wrong, but it seems to me that the current version of validate_pb2.py was copied from the Lyft-internal repo idlcode. To generate this PR I simply copied the latest version of validate_pb2.py from idlcode.

Possibly just copied the generated code from https://github.com/envoyproxy/protoc-gen-validate ?

That would be preferable, but as far as I can tell that repo doesn't contain the generated python code, only golang: https://github.com/envoyproxy/protoc-gen-validate/tree/main/validate

I'm not sure of anywhere else that the generated python exists, open to suggestions.

@milton0825
Copy link
Contributor

@hanzo you're right I meant the internal idlcode repo

@EngHabu
Copy link
Contributor

EngHabu commented May 6, 2021

can you run make generate and see what changes it makes?

@hanzo
Copy link
Contributor Author

hanzo commented May 6, 2021

can you run make generate and see what changes it makes?

I've been trying this but keep getting errors from mockery:

make generate
...
./generate_mocks.sh
+ mockery -dir=gen/pb-go/flyteidl/service/ -all -output=clients/go/admin/mocks
Error: unknown shorthand flag: 'i' in -ir=gen/pb-go/flyteidl/service/
Usage:
  mockery [flags]
...

Any idea what's going on there? Full command line history attached:

flyteidl error.txt

@hanzo
Copy link
Contributor Author

hanzo commented May 6, 2021

@EngHabu Seems like there's a step missing from this documentation? https://github.com/flyteorg/flyteidl#generate-code-from-protobuf Do I need to enter a docker container before running these make commands?

@EngHabu
Copy link
Contributor

EngHabu commented May 6, 2021

yes, mind running make download_tooling first?

@EngHabu
Copy link
Contributor

EngHabu commented May 6, 2021

essentially step 3 should be step1

@hanzo
Copy link
Contributor Author

hanzo commented May 6, 2021

yes, mind running make download_tooling first?

I did, the output is in the file I attached above.

@hanzo
Copy link
Contributor Author

hanzo commented May 6, 2021

I haven't figured out how to get make generate to succeed - it seems that a more recent version of mockery is being used which expects --dir instead of -dir. I'm not sure if these make commands are being run inside of the docker image as intended.

Fortunately, the files in my latest commit seem to get generated before make generate fails. Can a maintainer approve the workflow again so we can see if tests pass?

@akhurana001
Copy link
Contributor

@EngHabu does this look good ?

@milton0825
Copy link
Contributor

I see the tests passed. @hanzo can you sign the commit?

@EngHabu
Copy link
Contributor

EngHabu commented May 6, 2021

Can you take care of DCO? then we can merge it

hanzo and others added 3 commits May 6, 2021 15:02
@hanzo
Copy link
Contributor Author

hanzo commented May 6, 2021

Ok, I ran git rebase HEAD~3 --signoff and then git push --force-with-lease.

I was unable to get make generate to succeed so I asked my colleague @jcapote-lyft to run it on his machine. It succeeded for him but produced some seemingly unrelated changes, is that expected? c10b5d6

@EngHabu
Copy link
Contributor

EngHabu commented May 6, 2021

The CI system reruns the code generation. If it produced something different, it'll fail...

@hanzo
Copy link
Contributor Author

hanzo commented May 6, 2021

Makes sense. Looks like all checks are passing now, good to merge?

@akhurana001 akhurana001 self-requested a review May 6, 2021 22:21
@EngHabu EngHabu requested a review from akhurana001 May 6, 2021 22:21
@EngHabu EngHabu requested a review from kumare3 May 6, 2021 22:21
@EngHabu EngHabu merged commit 4deb86d into flyteorg:master May 6, 2021
@hanzo
Copy link
Contributor Author

hanzo commented May 10, 2021

Just to close the loop here - this PR was successful in resolving the collisions of the validate module that we were seeing in Lyft-internal repos. Thank you everyone for your help in getting this merged quickly! Very much appreciated.

Unfortunately this problem will resurface if idlcode gets updated to a newer version of protoc-gen-validate that contains breaking changes. We're looking into a better story for how repos like flyteidl can import the generated validate code - some possibilities include checking the generated code in to the protoc-gen-validate repo (the generated golang code is already there) or creating a secondary repo for the generated artifacts.

eapolinario pushed a commit that referenced this pull request Sep 8, 2023
* Update validate module to protoc-gen-validate 0.6.1

Signed-off-by: Hans Werner <[email protected]>

* include generated output

Signed-off-by: Hans Werner <[email protected]>

* update code

Signed-off-by: Hans Werner <[email protected]>

Co-authored-by: Julio Capote <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants