Skip to content

Conversation

@hill-daniel
Copy link

Currently, validation fails if the description of a property is changed in any way. The description validator ensures that it is possible to change the description of a property without failing validation.

Currently, validation fails if the description of a property is changed in any way. The description validator ensures that it is possible to change the description of a property without failing validation.
@everettraven
Copy link
Contributor

I'm a bit skeptical about adding a validator for this property. I think it is likely that the failure you are seeing here is related to the unhandledFailureMode being set to Closed, which means any property changes that we don't explicitly evaluate (in this case description is one of them) then it fails.

Maybe it makes sense to have the unhandledFailureMode option have the ability to be more granular and specify specific properties?

If I were to accept a validator for this property, I would expect it to not default to it being a breaking change.

@hill-daniel
Copy link
Author

hill-daniel commented Feb 27, 2025

I'm a bit skeptical about adding a validator for this property. I think it is likely that the failure you are seeing here is related to the unhandledFailureMode being set to Closed, which means any property changes that we don't explicitly evaluate (in this case description is one of them) then it fails.

Maybe it makes sense to have the unhandledFailureMode option have the ability to be more granular and specify specific properties?

If I were to accept a validator for this property, I would expect it to not default to it being a breaking change.

Yes, the description gets currently handled by the Default Validator.
Any changes to a description are a breaking change in FailureMode Closed, as you said.
So, do you suggest to adapt rather the failure mode with a sort of white list (just as a conceptual idea)?
Or should the description validator per default config be of type Enforcement None?

@everettraven
Copy link
Contributor

I'm a bit skeptical about adding a validator for this property. I think it is likely that the failure you are seeing here is related to the unhandledFailureMode being set to Closed, which means any property changes that we don't explicitly evaluate (in this case description is one of them) then it fails.
Maybe it makes sense to have the unhandledFailureMode option have the ability to be more granular and specify specific properties?
If I were to accept a validator for this property, I would expect it to not default to it being a breaking change.

Yes, the description gets currently handled by the Default Validator. Any changes to a description are a breaking change in FailureMode Closed, as you said. So, do you suggest to adapt rather the failure mode with a sort of white list (just as a conceptual idea)? Or should the description validator per default config be of type Enforcement None?

I like the idea of a white list for property changes, but that does make the project less opinionated as a whole which I think should start to be avoided.

For now, lets go with a validator that:

  • Allows all changes to the description property
  • Marks changes to the description property as handled
  • Has no configuration options

As I've been iterating on kubernetes/enhancements#5102 I've started leaning towards validators being less configurable and more opinionated. This is a pretty significant shift from how I initially implemented the validators but i think is the right direction to start going.

@k8s-triage-robot
Copy link

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

@linux-foundation-easycla
Copy link

CLA Not Signed

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 27, 2025
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Details

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-sigs/prow repository.

@everettraven
Copy link
Contributor

superseded by #16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants