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: remove [:alpha] protodoc tag and replace with annotation #18218

Merged
merged 6 commits into from
Sep 27, 2021

Conversation

mattklein123
Copy link
Member

This is a first step towards #17920

A single proto (kafka mesh) has been swapped from using the udpa
file_status annotation to the xds file_status annotation to avoid a
large amount of churn and a forthcoming migration of many alpha/wip
protos to non alpha/wip. The rest will be audited and swapped in
future PRs. This single one was done to make sure the doc machinary
works properly.

Risk Level: None, doc only
Testing: Manual review of docs
Docs Changes: Manually verified
Release Notes: N/A
Platform Specific Features: N/A

This is a first step towards #17920

A single proto (kafka mesh) has been swapped from using the udpa
file_status annotation to the xds file_status annotation to avoid a
large amount of churn and a forthcoming migration of many alpha/wip
protos to non alpha/wip. The rest will be audited and swapped in
future PRs. This single one was done to make sure the doc machinary
works properly.

Signed-off-by: Matt Klein <[email protected]>
@repokitteh-read-only repokitteh-read-only bot added api deps Approval required for changes to Envoy's external dependencies labels Sep 22, 2021
@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/.
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #18218 was opened by mattklein123.

see: more, trace.

Signed-off-by: Matt Klein <[email protected]>
@phlax
Copy link
Member

phlax commented Sep 22, 2021

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.

one suggestion, orthogonal of this migration PR (thank you so much for clean up) is we probably should ask that folks tagging things as WIP stick a TODO with their name on it or have a tracking issue so they don't forget it. Else I suspect many will get lost or forgotten about and git blame may lose track of who to poke about forward progress.

Basically it'd be nice to have tooling like we do for runtime guards to make sure things don't linger in WIP status. cc @adisuissa

@mattklein123
Copy link
Member Author

one suggestion, orthogonal of this migration PR (thank you so much for clean up) is we probably should ask that folks tagging things as WIP stick a TODO with their name on it or have a tracking issue so they don't forget it. Else I suspect many will get lost or forgotten about and git blame may lose track of who to poke about forward progress.

Yes, this makes sense. I will say that my next PR will start warning on this annotation, which I think will make people much more likely to complain/ask about it for follow up though. :)

@moderation
Copy link
Contributor

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Sep 22, 2021
@mattklein123
Copy link
Member Author

cc @adisuissa @htuch the breaking change detector is calling out my change. Let me know what I should do here? I assume we want to allow this type of change? At least addition?

@adisuissa
Copy link
Contributor

cc @adisuissa @htuch the breaking change detector is calling out my change. Let me know what I should do here? I assume we want to allow this type of change? At least addition?

This is because the new xds wip annotations PR (PR cncf/xds#11) isn't updated in the BSR yet.
@johanbrandhorst Can you please point me to how to sync the xds repo in the BSR (https://buf.build/beta/xds)?

Also opened #18228 because we need to modify our tool to allow breaking changes for WIP annotations.

@mattklein123
Copy link
Member Author

@adisuissa so can I ignore this failure for now?

@adisuissa
Copy link
Contributor

@adisuissa so can I ignore this failure for now?

I think it makes sense, so you won't be blocked by this problem.
One thing to be aware of is that we are going to see the same error in every PR that is rebased after this one, so you may want to disable the api_compat CI target in:

api_compat:
CI_TARGET: "bazel.api_compat"

until the BSR is updated.
That's unfortunate, but I think the only way to avoid such a mismatch in the future (for xds or any other package we depend on) is to use an alternative to buf.

@mattklein123
Copy link
Member Author

I don't fully understand what buf does, but once this merges, as long as there are no changes after this, won't it pass?

@adisuissa
Copy link
Contributor

buf looks at all the current api protos and their dependencies, and compares against the known package version in the BSR. At the moment the BSR xds package (https://buf.build/beta/xds/history) was last updated on Aug 9th, so your latest PR doesn't doesn't appear there.
Therefore the new annotation will not be recognized, and api_compat will continue to fail, until the BSR xds package is updated.

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.

Generally LGTM, thanks for working on this. A couple of minor comments.

tools/protodoc/protodoc.py Show resolved Hide resolved
tools/protodoc/protodoc.py Show resolved Hide resolved
@johanbrandhorst
Copy link

johanbrandhorst commented Sep 24, 2021

This is because the new xds wip annotations PR (PR cncf/xds#11) isn't updated in the BSR yet.
@johanbrandhorst Can you please point me to how to sync the xds repo in the BSR (https://buf.build/beta/xds)?

Hi again! Re: #18019 (comment), I'm not surprised 😁. As I explained there, these beta organization repositories are not actively maintained by us (Buf) anymore. Originally when launching the BSR beta we added these repositories to make it easy for users to experiment with dependency management, but we've adopted a more long term strategy recently in our first-party support for common libraries and we've narrowed the list to just:

  • buf.build/googleapis/googleapis
  • buf.build/gogo/protobuf
  • buf.build/envoyproxy/protoc-gen-validate

These are automatically synced at 11am ET every day by us, picking up any new changes from the last 24 hours.

The hope is that even these would eventually be adopted by their respective owners, but we are committing to maintaining them even if that does not happen. We're (hopefully understandably) reluctant to add other repositories here, but we recognize that envoy is a big open source project with constrained resources, so for this case we'd offer to pick up xds as well, since it's probably your most frequently changing dependency. The hope would be that eventually you would pick up that repository management (when/if the BSR proves itself valuable enough to you).

Taking a look at some of your other dependencies, there are a few other deprecated modules:

  • buf.build/beta/opencensus
  • buf.build/beta/prometheus
  • buf.build/beta/opentelemetry

These are probably updated much less frequently and with much less impact to you specifically, so it might not matter to you that they are not being synced anymore, but if it is a concern then ideally the project owners themselves could be convinced to create BSR repositories for them. It may be okay to wait until you notice any issues though.

Let us know how we can help and how you'd like to proceed, we want to make sure buf works well for you.

@mattklein123
Copy link
Member Author

Let us know how we can help and how you'd like to proceed, we want to make sure buf works well for you.

I still don't fully understand the problem here, but I will defer to @adisuissa on how he wants to proceed. In the meantime pending @htuch comments I will probably just disable the CI job.

@mattklein123
Copy link
Member Author

@htuch updated PTAL. In the next change I will actually warn when using any API with one of these annotations. After that I will begin converting some things away from alpha/wip that we know are used in prod.

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, thanks!

@mattklein123 mattklein123 merged commit f0f17a3 into main Sep 27, 2021
@mattklein123 mattklein123 deleted the remove_alpha_protodoc branch September 27, 2021 19:03
soulxu pushed a commit to soulxu/envoy that referenced this pull request Oct 16, 2021
…roxy#18218)

This is a first step towards envoyproxy#17920

A single proto (kafka mesh) has been swapped from using the udpa
file_status annotation to the xds file_status annotation to avoid a
large amount of churn and a forthcoming migration of many alpha/wip
protos to non alpha/wip. The rest will be audited and swapped in
future PRs. This single one was done to make sure the doc machinary
works properly.

Signed-off-by: Matt Klein <[email protected]>
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.

7 participants