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

chore: cleanup TODO list and alternatives considered for GEP 1709 #2051

Merged

Conversation

shaneutt
Copy link
Member

What type of PR is this?

/kind gep
/area conformance

What this PR does / why we need it:

In support of #1709 this cleans up our alternatives considered and TODO list. It moves some TODO items as blocking for post-experimental as opposed to blocking implementable based on our recent experience that suggests blocking at that level is not needed.

We previously considered mesh an "alternatives considered"
topic, as we didn't have tests available for mesh. We have
tests now, and we also already added "mesh" as a profile so
mesh is now in scope.
We decided to make ReferenceGrant part of profiles such as
HTTP for the time being, so no action needs to be taken on
this for the time being.
@shaneutt shaneutt added this to the v1.0.0 milestone May 22, 2023
@shaneutt shaneutt requested a review from mlavacca May 22, 2023 15:21
@k8s-ci-robot
Copy link
Contributor

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added kind/gep PRs related to Gateway Enhancement Proposal(GEP) do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 22, 2023
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2023
@shaneutt shaneutt removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label May 22, 2023
Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

Thanks @shaneutt!
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mlavacca, shaneutt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 3df4f36 into kubernetes-sigs:main May 22, 2023
@shaneutt shaneutt deleted the shaneutt/update-gep-1709-cleanup branch May 22, 2023 15:45
@shaneutt
Copy link
Member Author

Whoops, forgot to put a hold on it.

/cc @robscott @sunjayBhatia

Just give it a once over and make sure you're in agreement.

@sunjayBhatia
Copy link
Member

Whoops, forgot to put a hold on it.

/cc @robscott @sunjayBhatia

Just give it a once over and make sure you're in agreement.

lgtm

Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Just a few follow up questions, nothing major though

The following are items that we intend to resolve before we consider this GEP
`implementable`:

- We still need to sort out how we're going to display conformance results.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this being removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Check the commit history, it got moved down to "required to move past experimental" rather than "required to consider implementable. The thinking here is that the machinery we have provides a surface well enough for practically any display layer to be built on top, so it doesn't need to block (in fact, doesn't need to block GA) that we have a fancy display layer.

reach out to some frontend developers to come up with something really
solid prior to moving to `implementable`.
The following are items that **MUST** be resolved before we move past the
`provisioning/prototyping` status:
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nit: "Provisional"

Comment on lines -32 to -35
- Right now we've said that `GatewayClass` and `Gateway` tests will be
implicitly added to testing profiles were they are referenced, but we're
not sure yet whether this is going to work out for `ReferenceGrant` as we
know of at least one implementation that explicitly doesn't support it.
Copy link
Member

Choose a reason for hiding this comment

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

Why was this one removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Check the commit history, the commit includes an extended comment which explains: basically we decided to go the other way and implicitly added ReferenceGrant tests to profiles. It seems this might actually be fine for all implementations, if not we technically allow the feature to be disabled.

@mlavacca
Copy link
Member

Whoops, forgot to put a hold on it.

/cc @robscott @sunjayBhatia

Just give it a once over and make sure you're in agreement.

whoops, my bad. I should have put it on hold.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/gep PRs related to Gateway Enhancement Proposal(GEP) lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

5 participants