-
Notifications
You must be signed in to change notification settings - Fork 542
Add Istio report for 1.3.0 #3808
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
base: main
Are you sure you want to change the base?
Add Istio report for 1.3.0 #3808
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: keithmattix The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
I think the filename should have the implementation version in it
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.
It's the master branch, so would something like 1.27-dev work?
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.
It's the master branch, so would something like 1.27-dev work?
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.
Actually, I just saw the cilium filename contains main: https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/reports/v1.3.0/cilium-cilium/experimental-main-default-report.yaml
Feels like we should probably be consistent and use branch name here?
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.
Envoy and Airlock are using release tags, so we have a variety. For the reader's sake, I feel it would be better to report which release version is compliant.
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.
Feels like we should probably be consistent and use branch name here?
I would say yes, but what version of istio is it? like how would a user be able to reproduce?
We don't have a released version yet because the cadence is off. Is it so bad to have two dev versions and two release tags?
not sure I am following
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.
@LiorLieberman They would follow the instructions in README.md targeting the master branch. I'm not sure I understand your question
not sure I am following
I'm interpreting the PR feedback as "we should have this say 1.27-dev", but the cilium PR doesn't have a version; it just references the main branch. So if Istio's report matches Cilium's report, I'm not sure why there's an issue
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.
My feedback was mostly on the file name. Previous reports, istio included, had the version in the file names.
Implementations MUST report for a specific release version (e.g. v0.7.1) and not use branches or Git SHAs. Some exceptions will be made for initial reports to help make it easier for implementations to get started, but as we move to standard everyone should be reporting on specific releases.
This is what the docs suggest, see also https://gateway-api.sigs.k8s.io/geps/gep-1709/#reports-submission
I understand the report you generated is not a specific version, and I believe 1.27-dev works. Maybe @mlavacca can provide better guidance.
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.
FWIW we're in the process of releasing Istio 1.26.1 with Gateway API v1.3, so we expect to have a fixed release here before end of week.
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 you suggest we wait?
Signed-off-by: Keith Mattix II <[email protected]>
@keithmattix don't forget to update the v1.3 conformance table (#3810) when/as this PR merges |
70a021e
to
bb10051
Compare
Signed-off-by: Keith Mattix II <[email protected]>
What type of PR is this?
/kind documentation
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: