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

add ListenerConditionAccepted, update docs from "Detached" #1446

Merged

Conversation

mikemorris
Copy link
Contributor

@mikemorris mikemorris commented Oct 10, 2022

What type of PR is this?
/kind cleanup
/kind documentation
/kind api-change
/kind deprecation

What this PR does / why we need it:
Implements the switch from ListenerConditionDetached to a new ListenerConditionAttached, as recommended in GEP-1364.

Which issue(s) this PR fixes:

Fixes #1110, refs #1364

Does this PR introduce a user-facing change?:

Adds `ListenerConditionAccepted` and `ListenerReasonAccepted`
Deprecates `ListenerConditionDetached` and `ListenerReasonAttached`

Now that we've settled on the expected behavior, these expectations should likely be added to relevant conformance tests.

@k8s-ci-robot k8s-ci-robot added 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 Oct 10, 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.

Looking good. What are you looking for RE: kubebuilder annotations?

@mikemorris
Copy link
Contributor Author

@shaneutt I'm curious if something like the following can/should be used more granularly, like for these constants?

// +kubebuilder:deprecatedversion:warning="The v1alpha2 version of HTTPRoute has been deprecated and will be removed in a future release of the API. Please upgrade to v1beta1."

deprecated: true
deprecationWarning: The v1alpha2 version of HTTPRoute has been deprecated and
will be removed in a future release of the API. Please upgrade to v1beta1.

Or alternatively if there's some preferred convention like

// Deprecated: use GroupVersion instead.

@shaneutt
Copy link
Member

@shaneutt I'm curious if something like the following can/should be used more granularly, like for these constants?

// +kubebuilder:deprecatedversion:warning="The v1alpha2 version of HTTPRoute has been deprecated and will be removed in a future release of the API. Please upgrade to v1beta1."

deprecated: true
deprecationWarning: The v1alpha2 version of HTTPRoute has been deprecated and
will be removed in a future release of the API. Please upgrade to v1beta1.

Or alternatively if there's some preferred convention like

// Deprecated: use GroupVersion instead.

Ah, now I see what you're getting at. I am only aware of the kubebuilder:deprecatedversion tag as per the kubebuilder CRD generation documentation, which does not apply here because it marks the entire version as deprecated. I am not aware of any way to call a specific field deprecated. I think we can just use the godoc you've already provided explaining that it's deprecated for now, maybe we can open/find an upstream issue to add something that automates this for us in kubebuilder later.

@mikemorris
Copy link
Contributor Author

mikemorris commented Oct 11, 2022

Removed TODOs and switched deprecation warnings from passive to active voice, think this should be ready to go now.

@robscott
Copy link
Member

Thanks @mikemorris!

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikemorris, robscott

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2022
@k8s-ci-robot k8s-ci-robot merged commit 1d0ac97 into kubernetes-sigs:main Oct 11, 2022
@mikemorris mikemorris deleted the listener-condition-attached branch October 12, 2022 17:18
//
// Controllers may raise this condition with other reasons,
// but should prefer to use the reasons listed above to improve
// interoperability.
ListenerConditionAccepted ListenerConditionType = "Accepted"
Copy link
Contributor

Choose a reason for hiding this comment

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

The docs here (L330) are wrong now, the polarity should be inverted

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. 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
None yet
Development

Successfully merging this pull request may close these issues.

"good" state for ListenerStatus feels logically inverted, difficult to understand
5 participants