-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
docs: remove annotations-based l7 visibility #28449
docs: remove annotations-based l7 visibility #28449
Conversation
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 ✅
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.
Looks great! I just have a couple of suggestions. Also, can you add a note about this in the upgrade guide: https://github.com/cilium/cilium/blob/main/Documentation/operations/upgrade.rst#115-upgrade-notes?
4a71ef0
to
2184b06
Compare
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.
Changes look great, thanks! I have a couple of suggestions for you, and then we'll be good to go.
64dc78b
to
76616db
Compare
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.
LGTM! Thank you!
@networkop this fell out of date, please rebase. @cilium/proxy please review |
👍, my review can be counted from proxy team. I had a chat with Michael offline before as well. |
44940bc
to
fa01074
Compare
Signed-off-by: Michael Kashin <michael.kashin@isovalent.com>
fa01074
to
0405731
Compare
01fcacc
to
dee23ee
Compare
@networkop Given its docs we don't need to run the whole test suite, is this "ready-to-merge"? |
@dylandreimerink yes, it's ready to merge from my pov. |
@dylandreimerink CI should detect that it's only docs and skip the other tests. This way we can just run CI on every PR, and not debate about ready-to-merge from CI perspective. If this is triggering full CI then we can iterate on the detection, I would consider that a CI bug. I'm trying to get us to a point where we don't try to make judgement calls about whether to skip CI or not, because that often becomes a point of failure in the process where we let buggy logic into the tree. |
/test |
L7-based annotations have had little adoption and are causing a lot of bugs. This change is to move them to the background in the docs and bring forward the more standard way of enabling L7 visibility (CNP)