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

api: include v1alpha1 in proto_sync.py #9175

Merged
merged 4 commits into from
Dec 10, 2019

Conversation

derekargueta
Copy link
Member

@derekargueta derekargueta commented Nov 30, 2019

Description: Follow-up to #8133, these likely escaped detection since they're versioned "v1alpha1" and the cloning script only searches for the last major version. This should be a one-off to get these caught up so I'm not modifying the tooling to accommodate these.
Risk Level: low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Derek Argueta [email protected]

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9175 was opened by derekargueta.

see: more, trace.

@htuch
Copy link
Member

htuch commented Dec 2, 2019

@derekargueta I think we do need to have the tools do this, since these APIs might gain new fields before we do the v3 cut. I think it's fairly easy to do, check out tools/type_whisperer/typedb_gen.py and tools/proto_sync.py, look for references to v2alpha.

@derekargueta
Copy link
Member Author

I didn't want to add one-off logic but since we already handling vNalphaN for v2 in tools/proto_sync.py I feel less bad :) will push an update.

@htuch htuch added the waiting label Dec 4, 2019
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9175 was synchronize by derekargueta.

see: more, trace.

Signed-off-by: Derek Argueta <[email protected]>
@derekargueta
Copy link
Member Author

@htuch tooling updated. I believe it's is working as expected. When running with some debug prints, I verified it picks up the v1alpha1 bazel targets but hits this line because (IIUC) there are no deprecated fields in v1alpha1 for these two protos so there's nothing to modify.

@htuch
Copy link
Member

htuch commented Dec 5, 2019

@derekargueta ah yeah. So, I think your tool changes make sense, but then we shouldn't have the actual protos in v3alpha. We're only upgrading packages that have either had a breaking change or transitively depend on a package that has been upgraded. We basically have a DAG of versions.

@derekargueta derekargueta changed the title api: add zookeeper and mysql to v3alpha api: include v1alpha1 in proto_sync.py Dec 10, 2019
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks!

@htuch htuch merged commit 8cf6e56 into envoyproxy:master Dec 10, 2019
@derekargueta derekargueta deleted the dereka/v3-stragglers branch December 11, 2019 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants