-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: introduce WiP file-level annotations. #9971
Conversation
This is the new style for indicating a file is WiP and subject to breaking changes. Rather than rely on alpha major versions, which are coarse grained and introduce migration difficulties for operators, we use a file-level annotation. Risk level: Low Testing: API/docs build, manual inspection of docs. Fixes envoyproxy#9769. Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking this up. Check format?
/wait
tools/check_format.py
Outdated
@@ -766,11 +766,11 @@ def checkFormat(file_path): | |||
if isBuildFile(file_path) or isSkylarkFile(file_path) or isWorkspaceFile(file_path): | |||
if try_to_fix: | |||
error_messages += fixBuildPath(file_path) | |||
error_messages += checkBuildPath(file_path) | |||
#error_messages += checkBuildPath(file_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert? (I assume you are debugging the same issue I am dealing with)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (yes)
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks. A few small questions.
/wait
This places the filter in the correct [v3 package hierarchy](#package-organization). | ||
1. If this is still WiP and subject to breaking changes, import | ||
`udpa/annotations/status.proto` and set `option (udpa.annotations.file_status).work_in_progress = true;`. | ||
1. Add a reference to the v2 extension config in (1) in [api/docs/BUILD](docs/BUILD). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was confusion about this recently, but how are the v3 docs being built? Doesn't something have to be added for v3 also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#9719 added the mechanism: proto_format.sh runs tools generating api/BUILD, which contains v2_protos
and v3_protos
there, the source of proto_format.sh is api/docs/BUILD
which only needs v2 to be added. So v3 references doesn't have to be added now.
This places the filter in the correct [v3 package hierarchy](#package-organization). | ||
3. Add a reference to the v2 extension config in (1) in [api/docs/BUILD](docs/BUILD). | ||
4. Run `./tools/proto_format fix`. This should regenerate the `BUILD` file, | ||
1. Add to the v2 extension config proto a file level `option (udpa.annotations.file_migrate).move_to_package = "envoy.extensions.filters.http.foobar.v3";`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per your comment about about vNalpha, I'm confused. Are we back to saying that it's OK to use alpha for a proto? If so do we need this annotation? Or do we prefer to do it also? It seems redundant? I thought we had this annotation because we couldn't do the alpha. Either way can you potentially clarify the docs and have a worked alpha example below if that is what we are doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this addresses my own comment here. The annotations seems redundant now, but it has doc effect, also it will help when we stabilize v4alpha to v4. When we boost v4alpha, we don't have information whether a proto is boosted from v3alpha
or v3
, when we stabilize v4alpha
to v4
we can exclude protos with this annotation.
This also gives a chance to stabilize proto during major upgrade: i.e. removing annotation from an v3alpha proto file will make it to v4 during the major upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK that's fine with me if you think it works. Can you update the docs a bit to have an example of both cases? I think that would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattklein123 done and add link to example filters.
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
This is the new style for indicating a file is WiP and subject to
breaking changes.
Risk Level: Low
Testing: CI
Docs Changes: api/STYLE
Release Notes: N/A
Fixes #9769.