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

Consider "false" value when evaluating "bool" annotations #7434

Closed
jackfrancis opened this issue Oct 20, 2022 · 9 comments
Closed

Consider "false" value when evaluating "bool" annotations #7434

jackfrancis opened this issue Oct 20, 2022 · 9 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/backlog Higher priority than priority/awaiting-more-evidence.

Comments

@jackfrancis
Copy link
Contributor

User Story

As a user I would like existing annotations annotations explicitly set to "false" to be respected as false in Cluster API.

Detailed Description

For annotations where we simply look for the presence of a well-known key to indicate true/false, we should consider enabling support for explicit "false" indicators. The most obvious is the value of "false" in the value of the existent annotation.

[A clear and concise description of what you want to happen.]

Anything else you would like to add:

This precedent was established as a part of this PR: #7107

[Miscellaneous information that will assist in solving the issue.]

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 20, 2022
@sbueringer
Copy link
Member

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 20, 2022
@DiptoChakrabarty
Copy link
Member

/assign

@vincepri
Copy link
Member

Are there some examples that supporting false as a value is beneficial to user? Same for #7107, what's the benefit vs removing the annotation completely?

@jackfrancis
Copy link
Contributor Author

@vincepri the idea we discussed when this issue was filed was that in the (rare) event where a new user w/ pre-existing Kubernetes infrastructure adopts Cluster API, they may already be using CAPI annotations for previous use in a negative sense, and if so we should respect that.

I don't remember the exact conversation, but (despite the fact that I filed this issue!) I would prefer that CAPI standardize to requiring significance for both key + value in any capi-specific annotations to avoid collisions w/ pre-existing usage of those annotations across the Kubernetes ecosystem. Because there's no standard set of "Kubernetes annotations", we can't guarantee uniqueness when we come up w/ a new, CAPI-specific annotation.

In any event this issue is a good thread to have this conversation for folks who are interested.

@sbueringer
Copy link
Member

sbueringer commented Jan 25, 2023

But aren't CAPI specific annotations namespaced to our namespace? I think that should guarantee that nobody else defined them before.

Nobody else should define annotations in our namespace

@k8s-triage-robot
Copy link

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Jan 25, 2024
@fabriziopandini
Copy link
Member

/priority backlog

@k8s-ci-robot k8s-ci-robot added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Apr 11, 2024
@fabriziopandini
Copy link
Member

The Cluster API project currently lacks enough active contributors to adequately respond to all issues and PRs.
I don't see folks willing to commit to this

For annotations where we simply look for the presence of a well-known key to indicate true/false, we should consider enabling support for explicit "false" indicators.

We can reopen if conditions arises + someone volounteers
/close

@k8s-ci-robot
Copy link
Contributor

@fabriziopandini: Closing this issue.

In response to this:

The Cluster API project currently lacks enough active contributors to adequately respond to all issues and PRs.
I don't see folks willing to commit to this

For annotations where we simply look for the presence of a well-known key to indicate true/false, we should consider enabling support for explicit "false" indicators.

We can reopen if conditions arises + someone volounteers
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants