Skip to content

Comments

remove unnecessary file level not unimplemented hide annotation#18924

Merged
htuch merged 2 commits intoenvoyproxy:mainfrom
wbpcode:fix-doc
Nov 9, 2021
Merged

remove unnecessary file level not unimplemented hide annotation#18924
htuch merged 2 commits intoenvoyproxy:mainfrom
wbpcode:fix-doc

Conversation

@wbpcode
Copy link
Member

@wbpcode wbpcode commented Nov 6, 2021 •
edited
Loading

Commit Message: remove unnecessary file level 'not unimplemented hide' annotation
Additional Description:

Remove the unnecessary file level 'not-implemented-hide' annotation. I found these annotations during the development of PR #18923 . But it seems that they should not have this annotation.

If this is a misunderstanding on my part, please close this PR.

Risk Level: Doc Only.
Testing: N/A.
Docs Changes: N/A.
Release Notes: N/A.

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Nov 6, 2021

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #18924 was opened by wbpcode.

see: more, trace.

@wbpcode wbpcode mentioned this pull request Nov 8, 2021
Merged
@phlax
Copy link
Member

phlax commented Nov 8, 2021

docs for this PR are rendered here: https://storage.googleapis.com/envoy-pr/18924/docs/index.html

phlax
phlax previously approved these changes Nov 8, 2021
View reviewed changes
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

afaict, removing these makes no difference - altho i guess with #18923 it will make a difference

https://storage.googleapis.com/envoy-pr/3b43372/docs/api-v3/service/discovery/v3/ads.proto.html

https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/discovery/v3/ads.proto.html

so, lgtm

cc @htuch for final approval

adisuissa
adisuissa reviewed Nov 8, 2021
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up!
IMHO: I think ADS should be ok to remove, but for LEDS let's keep it hidden until the docs are ready.

api/envoy/service/endpoint/v3/leds.proto
option java_generic_services = true;
option (udpa.annotations.file_status).package_version_status = ACTIVE;

// [#not-implemented-hide:]
Copy link
Contributor

@adisuissa adisuissa Nov 8, 2021

Choose a reason for hiding this comment

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

I would keep this as hidden until #18429 is finalized

Copy link
Member Author

@wbpcode wbpcode Nov 8, 2021

Choose a reason for hiding this comment

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

get it.

minor update
2ca9cf7 
Signed-off-by: wbpcode <[email protected]>
@wbpcode
Copy link
Member Author

wbpcode commented Nov 9, 2021

cc @adisuissa @htuch

adisuissa
adisuissa approved these changes Nov 9, 2021
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

LGTM.
Leaving for @htuch to make sure I'm not missing anything.

@repokitteh-read-only repokitteh-read-only bot removed the api label Nov 9, 2021
htuch
htuch approved these changes Nov 9, 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, thanks!

@htuch
Copy link
Member

htuch commented Nov 9, 2021

/lgtm v2-freeze

@htuch htuch merged commit e9203e9 into envoyproxy:main Nov 9, 2021
mpuncel added a commit to mpuncel/envoy that referenced this pull request Nov 10, 2021
@mpuncel
Merge branch 'main' into mpuncel/hc-after-sds-init-manager
780f15c 
* main: (71 commits)
  bazel: fix macOS build (envoyproxy#18920)
  http: switching from 100 to 1xx (envoyproxy#18904)
  grpc: implement BufferedAsyncClient for bidirectional gRPC stream (envoyproxy#18129)
  bazel: add repository arg to benchmark_test (envoyproxy#18795)
  rocketmq_proxy: Improvement for map find (envoyproxy#18909)
  tls: unit test: spiffe signed by intermediate cert (envoyproxy#18914)
  Test for FilterConfigPerRoute dtor called on worker thread. (envoyproxy#18927)
  deps: Bump `com_google_protobuf` -> 3.19.1 (envoyproxy#18930)
  deps: Bump `com_googlesource_code_re2` -> 2021-11-01 (envoyproxy#18933)
  cvescan: Move cvescan data to yaml (envoyproxy#18947)
  remove unnecessary file level not unimplemented hide annotation (envoyproxy#18924)
  test: moving echo test (envoyproxy#18938)
  test: fixing a test flake (envoyproxy#18899)
  deps: Revert pyparsing bump (envoyproxy#18946)
  deps: Bump `build_bazel_rules_apple` -> 0.32.0 (envoyproxy#18932)
  deps: Bump `com_github_bazelbuild_buildtools` -> 4.2.3 (envoyproxy#18931)
  build(deps): bump pycparser from 2.20 to 2.21 in /tools/dependency (envoyproxy#18936)
  quic: supporting connections with zero initial available streams (envoyproxy#18775)
  test: moving proxy proto (envoyproxy#18939)
  build(deps): bump pyparsing from 3.0.4 to 3.0.5 in /tools/dependency (envoyproxy#18937)
  ...

Signed-off-by: Michael Puncel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adisuissa adisuissa adisuissa approved these changes

@phlax phlax phlax left review comments

+1 more reviewer

@htuch htuch htuch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@adisuissa adisuissa

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wbpcode @phlax @htuch @adisuissa