add the OPENSHIFT_REQUIRED_FEATURESET env var and openshift:enable:featureSet marker #11
Conversation
openshift-ci
bot
added
the
do-not-merge/hold
There was a problem hiding this comment.
Got a controller-gen binary checked in, please drop that
Do we need to be able to support a field being present in multiple FeatureSets? In Kubebuilder you can create separated lists of values, like the enum marker, perhaps we want to support this too by copying that pattern?
pkg/crd/markers/patch_validation.go
Outdated
|
|
||
| import "sigs.k8s.io/controller-tools/pkg/markers" | ||
|
|
||
| const OpenShiftFeatureSetMarkerName = "openshift:enable:featureSet" |
There was a problem hiding this comment.
Kubebuilder uses PascalCase for the third element, I think we should be consistent
| const OpenShiftFeatureSetMarkerName = "openshift:enable:featureSet" | |
| const OpenShiftFeatureSetMarkerName = "openshift:enable:FeatureSet" |
There was a problem hiding this comment.
Ah, coding via side-effect. change this case and we end up down a different code path that fails in controller-tools. Such a pain. I'll chase it and prepare for sadness.
pkg/crd/patch_schema.go
Outdated
| var RequiredFeatureSet = os.Getenv("OPENSHIFT_REQUIRED_FEATURESET") | ||
|
|
||
| // mayHandleField returns true if the field should be considered by this invocation of the generator. | ||
| // Right now, the only sip is based on the featureset marker. |
pkg/schemapatcher/patch_gen.go
Outdated
| return true | ||
| } | ||
|
|
||
| manifestFeatureSet := manifest.GetAnnotations()["release.openshift.io/feature-set"] |
There was a problem hiding this comment.
Can GetAnnotations ever return a nil?
There was a problem hiding this comment.
Can GetAnnotations ever return a nil?
Yes, it can. You can read values from a nil map: https://go.dev/play/p/hHg_-G6RpKE
pkg/schemapatcher/patch_gen.go
Outdated
| if len(RequiredFeatureSet) == 0 { | ||
| if len(manifestFeatureSet) == 0 { | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Is this needed?
The output of this function is currently
| required | manifest | required == manifest | output |
|---|---|---|---|
"" |
"" |
true | true |
"" |
foo |
false | false |
| foo | foo | true | true |
| foo | bar | false | false |
I think we can drop these lines unless there's something surprising happening here?
deads2k
force-pushed
the
techpreview-01-for-old
branch
2 times, most recently
from
0ab0804
to
2e4a7b9
Compare
|
One outstanding question else I'm happy with this |
deads2k
force-pushed
the
techpreview-01-for-old
branch
from
2e4a7b9
to
1891174
Compare
|
/lgtm |
|
@JoelSpeed: changing LGTM is restricted to collaborators In response to this:
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. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
This allows openshift/api to have separate sets of manifests for different featuresets.
/hold
holding for discussion with @JoelSpeed