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.

Sign up for GitHub

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

Jump to bottom

api: force migrate v3alpha -> v3 #18392

Merged
merged 4 commits into from
Oct 4, 2021
Merged

api: force migrate v3alpha -> v3 #18392

merged 4 commits into from
Oct 4, 2021

Conversation

mattklein123
Copy link
Member

@mattklein123 mattklein123 commented Oct 4, 2021 •
edited
Loading

Also adjust wip status for some things that are known to be used in
production.

Fixes #17920

Risk Level: High (for code, low, but there will be unhappy v3alpha users surely.)
Testing: Existing tests
Docs Changes: N/A
Release Notes: Added
Platform Specific Features: N/A

@mattklein123
api: force migrate v3alpha -> v3
9026c94 
Also adjust wip status for some things that are known to be used in
production.

Fixes #17920

Signed-off-by: Matt Klein <mklein@lyft.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #18392 was opened by mattklein123.

see: more, trace.

@mattklein123
Copy link
Member Author

cc @envoyproxy/api-shepherds

@mattklein123
fix
3b9ac3a 
Signed-off-by: Matt Klein <mklein@lyft.com>
htuch
htuch reviewed Oct 4, 2021
View reviewed changes
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.

I think risk should be "high", in the sense this is likely to break a lot of people who are not following the letter of the law on API stability policy. Caveat emptor, I know, but if I was scanning commit logs for changes to pay attention to, this is one.

api/envoy/extensions/cache/simple_http_cache/v3/config.proto
@@ -1,10 +1,10 @@
syntax = "proto3";

package envoy.extensions.cache.simple_http_cache.v3alpha;
package envoy.extensions.cache.simple_http_cache.v3;
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's totally valid to break stuff since these are alpha APIs. One alternative we have though is to clone v3alpha to v3 and just not support v3alpha in Envoy. I'm not sure how useful that would be, but if other API clients depend on alpha protos, that could be less bad. @markdroth any opinions here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have any strong opinion on this. gRPC doesn't use any alpha APIs, and I don't think that GCP depends on any of these APIs either.

In principle, I guess I agree that not forcibly breaking anyone who is depending on the alpha protos might be a bit more user-friendly, but at the same time, the whole point of alpha is to be able to make breaking changes, so... I can see both sides of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless there are strong objections I'm just going to suggest we pull the bandaid here and get it done. If we receive extreme complaining we could potentially add the files back but let's see what happens?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds fine to me.

@mattklein123
Copy link
Member Author

I think risk should be "high"

Updated

@alyssawilk alyssawilk added this to the 1.20.0 milestone Oct 4, 2021
htuch
htuch reviewed Oct 4, 2021
View reviewed changes
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.

LGTM contingent on other API shepherds (in particular @markdroth) sign-off.

markdroth
markdroth reviewed Oct 4, 2021
View reviewed changes
EXTENSION_POLICY.md Outdated Show resolved Hide resolved
api/envoy/extensions/cache/simple_http_cache/v3/config.proto
@@ -1,10 +1,10 @@
syntax = "proto3";

package envoy.extensions.cache.simple_http_cache.v3alpha;
package envoy.extensions.cache.simple_http_cache.v3;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have any strong opinion on this. gRPC doesn't use any alpha APIs, and I don't think that GCP depends on any of these APIs either.

In principle, I guess I agree that not forcibly breaking anyone who is depending on the alpha protos might be a bit more user-friendly, but at the same time, the whole point of alpha is to be able to make breaking changes, so... I can see both sides of this.

@mattklein123
Merge remote-tracking branch 'origin/main' into migrate_alpha
0c5034e 
Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123
comment
7516afd 
Signed-off-by: Matt Klein <mklein@lyft.com>
@markdroth
Copy link
Contributor

/lgtm api

@repokitteh-read-only repokitteh-read-only bot removed the api label Oct 4, 2021
alyssawilk
alyssawilk approved these changes Oct 4, 2021
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

(given the dual shephard sign-off I'm fine with the actual changes)

@mattklein123 mattklein123 merged commit 54b686b into main Oct 4, 2021
@mattklein123 mattklein123 deleted the migrate_alpha branch October 4, 2021 20:44
@mykhailoaksani mykhailoaksani mentioned this pull request Oct 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htuch htuch htuch left review comments

@markdroth markdroth markdroth left review comments

@alyssawilk alyssawilk alyssawilk approved these changes

@antoniovicente antoniovicente Awaiting requested review from antoniovicente

@jmarantz jmarantz Awaiting requested review from jmarantz jmarantz is a code owner

@lizan lizan Awaiting requested review from lizan

@snowp snowp Awaiting requested review from snowp

@yanavlasov yanavlasov Awaiting requested review from yanavlasov yanavlasov is a code owner

Assignees

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
1.20.0
Development

Successfully merging this pull request may close these issues.

Clarify API versioning for alpha APIs
4 participants
@mattklein123 @markdroth @htuch @alyssawilk