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

replace Attached condition with Accepted in HTTPRoute docs #1382

Merged
merged 1 commit into from
Sep 26, 2022

Conversation

skriss
Copy link
Contributor

@skriss skriss commented Sep 7, 2022

The HTTPRoute docs have several references to
an "Attached" condition, however this condition
does not exist for routes. Replaces these
references with the appropriate "Accepted"
condition.

Signed-off-by: Steve Kriss krisss@vmware.com

What type of PR is this?
/kind documentation

What this PR does / why we need it:

The HTTPRoute docs have several references to
an "Attached" condition, however this condition
does not exist for routes. Replaces these
references with the appropriate "Accepted"
condition.

See https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io%2fv1beta1.RouteConditionType for the list of valid Route condition types and reasons.

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 7, 2022
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 7, 2022
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

So from what I can tell, this appears to be right: we've had this wrong for some time as the Accepted condition exists for HTTPRoute but not Attached (which is only a Gateway listener condition elsewhere). Approved, but I would like another maintainer to just double check me and give the LGTM because I can't shake the feeling like I might be missing something 🤔

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 7, 2022
@youngnick
Copy link
Contributor

This change is one hundred percent correct as per the current spec, but I'm currently working on a status rewrite which proposes we move to Attached everywhere. So maybe this should wait until I finish that up and we discuss it?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2022
@mikemorris
Copy link
Contributor

This content likely needs to be revisited now that #1383 has merged.

@skriss
Copy link
Contributor Author

skriss commented Sep 26, 2022

So this was just trying to fix a bug in the v0.5.0 spec/docs, but I guess at this point it's probably missed the boat for inclusion in v0.5.1? If so, and the broader v0.6.0 conditions changes will cover this, then we can just close this one out and live with the v0.5.x docs bug.

The HTTPRoute docs have several references to
an "Attached" condition, however this condition
does not exist for routes. Replaces these
references with the appropriate "Accepted"
condition.

Signed-off-by: Steve Kriss <krisss@vmware.com>
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 26, 2022
@skriss
Copy link
Contributor Author

skriss commented Sep 26, 2022

Rebased in case we want to include it in v0.5.1, if not, and will be covered by larger v0.6 changes, feel free to close.

@robscott
Copy link
Member

Thanks @skriss! If you follow up with a cherry-pick PR into release-0.5, I'll add it to the v0.5.1 changelog.

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott, shaneutt, skriss

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 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2022
@k8s-ci-robot k8s-ci-robot merged commit 2738bba into kubernetes-sigs:main Sep 26, 2022
k8s-ci-robot added a commit that referenced this pull request Sep 26, 2022
(release-0.5 cherrypick of #1382) replace Attached condition with Accepted in HTTPRoute docs
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants