WIP: ⚠️ updates from api audit #1404

grokspawn · 2024-10-22T19:12:01Z

Description

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2024-10-22T19:12:18Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`7a14b4d`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/672a6b038990070008b87cc0
😎 Deploy Preview	https://deploy-preview-1404--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-10-22T19:19:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.31%. Comparing base (6f42274) to head (b7e2f01).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1404   +/-   ##
=======================================
  Coverage   73.31%   73.31%           
=======================================
  Files          42       42           
  Lines        3166     3166           
=======================================
  Hits         2321     2321           
  Misses        659      659           
  Partials      186      186

Flag	Coverage Δ
e2e	`54.83% <ø> (ø)`
unit	`52.55% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

everettraven · 2024-10-22T20:27:05Z

api/v1alpha1/clusterextension_types.go

@@ -496,6 +507,7 @@ type ClusterExtensionStatus struct {
 	// +patchStrategy=merge
 	// +listType=map
 	// +listMapKey=type
+	// +kubebuilder:validation:Required


This should be optional?

Yes. A copy/paste error.

everettraven · 2024-10-22T20:27:44Z

config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml

+            required:
+            - conditions


I would have expected no diff in our generated CRD by only adding the required tags to the already required fields

Ditto! And yet ...
It's even more apparently-empty in the generated docs!

I left another comment that I think the conditions should be optional. We include omitempty in the JSON tag. If it should be required then this change is fine.

perdasilva · 2024-11-05T13:47:26Z

api/v1alpha1/clusterextension_types.go

@@ -258,7 +264,7 @@ type CatalogSource struct {
 	// For more information on semver, please see https://semver.org/
 	//
 	//+kubebuilder:validation:MaxLength:=64
-	//+kubebuilder:validation:Pattern=`^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$`
+	//+kubebuilder:validation.XValidation:rule="self.matches(r'^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$')"", message="invalid version expression in the catalog source"


Is the error message correct here?

We don't need to say "in the catalog source" because the error emitted by the validator includes the parent path leading up to this field.

Suggested change

//+kubebuilder:validation.XValidation:rule="self.matches(r'^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$')"", message="invalid version expression in the catalog source"

//+kubebuilder:validation.XValidation:rule="self.matches(r'^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$')"", message="invalid version expression"

The error message here definitely seems a bit vague to me. Could we update the message to also include some helpful information for crafting a valid version expression?

Signed-off-by: Jordan Keister <jordan@nimblewidget.com>

everettraven · 2024-11-06T14:56:56Z

api/v1alpha1/clusterextension_types.go

@@ -80,14 +82,15 @@ const SourceTypeCatalog = "Catalog"
 type SourceConfig struct {


I believe we need to update the CEL validation here to match the changes in https://github.com/operator-framework/catalogd/pull/443/files#diff-d446855f2187e6d61c4719c3202fcb21e4aea9b09e09f9e1d01419f341d8fc9fR233

It looks like Joel had requested this as well: https://github.com/openshift/api/pull/2067/files#r1812412946

everettraven · 2024-11-06T14:58:27Z

api/v1alpha1/clusterextension_types.go

 	//
 	// When this field is set to "Catalog", information for determining the appropriate
 	// bundle of content to install will be fetched from ClusterCatalog resources existing
 	// on the cluster. When using the Catalog sourceType, the catalog field must also be set.
 	//
 	// +unionDiscriminator
 	// +kubebuilder:validation:Enum:="Catalog"
+	// +kubebuilder:validation:Required
 	SourceType string `json:"sourceType"`

 	// catalog is used to configure how information is sourced from a catalog. This field must be defined when sourceType is set to "Catalog",


Should probably update this to reflect any updates we make to the CEL validation following my comment on this structs validations

everettraven · 2024-11-06T15:02:17Z

api/v1alpha1/clusterextension_types.go

@@ -130,6 +133,7 @@ type ClusterExtensionInstallConfig struct {
 	//+kubebuilder:validation:Pattern:=^[a-z0-9]([-a-z0-9]*[a-z0-9])?$
 	//+kubebuilder:validation:MaxLength:=63
 	//+kubebuilder:validation:XValidation:rule="self == oldSelf",message="namespace is immutable"
+	//+kubebuilder:validation:Required
 	Namespace string `json:"namespace"`


There were a few comments from Joel related to the GoDoc for this field that I don't see reflected here. Did we decide against accepting those suggestions or making adjustments to clarify some of the questions asked?

Ref:

https://github.com/openshift/api/pull/2067/files#r1812416875

https://github.com/openshift/api/pull/2067/files#r1812425452

https://github.com/openshift/api/pull/2067/files#r1812430084

https://github.com/openshift/api/pull/2067/files#r1812416228

everettraven · 2024-11-06T15:02:35Z

api/v1alpha1/clusterextension_types.go

@@ -130,6 +133,7 @@ type ClusterExtensionInstallConfig struct {
 	//+kubebuilder:validation:Pattern:=^[a-z0-9]([-a-z0-9]*[a-z0-9])?$


Should we use CEL here instead of pattern validation?

joelanford · 2024-11-06T15:06:33Z

api/v1alpha1/clusterextension_types.go

@@ -69,6 +70,7 @@ type ClusterExtensionSpec struct {
 	//    namespace: example-namespace
 	//    serviceAccount:
 	//      name: example-sa
+	// +kubebuilder:validation:Required


Add an extra new line above the Required comment marker?

joelanford · 2024-11-06T15:11:34Z

api/v1alpha1/clusterextension_types.go

 	// This field is required when the spec.install.preflight.crdUpgradeSafety field is
 	// specified.


I think I saw a comment from Joel about not needing the conditional statement here. We can just say "enforcement is required" unconditionally (it is implicit that it being required is conditional on the parent struct being specified).

everettraven · 2024-11-06T15:11:36Z

api/v1alpha1/clusterextension_types.go

@@ -258,14 +264,14 @@ type CatalogSource struct {
 	// For more information on semver, please see https://semver.org/
 	//
 	//+kubebuilder:validation:MaxLength:=64
-	//+kubebuilder:validation:Pattern=`^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$`
+	//+kubebuilder:validation.XValidation:rule="self.matches(r'^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$')"", message="invalid version expression in the catalog source"


Suggested change

//+kubebuilder:validation.XValidation:rule="self.matches(r'^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$')"", message="invalid version expression in the catalog source"

//+kubebuilder:validation:XValidation:rule="self.matches(r'^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$')"", message="invalid version expression in the catalog source"

everettraven · 2024-11-06T15:14:45Z

api/v1alpha1/clusterextension_types.go

@@ -140,6 +144,7 @@ type ClusterExtensionInstallConfig struct {
 	// the ServiceAccount provided via this field should be configured with the
 	// appropriate permissions to perform the necessary operations on all the
 	// resources that are included in the bundle of content being applied.
+	//+kubebuilder:validation:Required
 	ServiceAccount ServiceAccountReference `json:"serviceAccount"`

 	// preflight is an optional field that can be used to configure the preflight checks run before installation or upgrade of the content for the package specified in the packageName field.


Do we need to be more descriptive about what the default preflight checks are?

Context: https://github.com/openshift/api/pull/2067/files#r1812435988

everettraven · 2024-11-06T15:16:27Z

api/v1alpha1/clusterextension_types.go

@@ -181,6 +186,7 @@ type CatalogSource struct {
 	//+kubebuilder:validation:MaxLength:=253
 	//+kubebuilder:validation:Pattern:=^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
 	//+kubebuilder:validation:XValidation:rule="self == oldSelf",message="packageName is immutable"
+	//+kubebuilder:validation:Required
 	PackageName string `json:"packageName"`


Similar question regarding Joel having comments on the GoDoc for this field - are we intentionally not changing the GoDoc?

joelanford · 2024-11-06T15:16:55Z

internal/applier/helm.go

+func shouldSkipPreflight(preflight Preflight, ctx context.Context, ext *ocv1alpha1.ClusterExtension, state string) bool {
+	l := log.FromContext(ctx)
+	if ext.Spec.Install.Preflight != nil && ext.Spec.Install.Preflight.CRDUpgradeSafety != nil {
+		if _, ok := preflight.(*crdupgradesafety.Preflight); ok {


Maybe move this line out to the calling function, probably a more appropriate place for it, and might solve the linter concern around nested blocks.

everettraven · 2024-11-06T15:17:09Z

api/v1alpha1/clusterextension_types.go

@@ -181,6 +186,7 @@ type CatalogSource struct {
 	//+kubebuilder:validation:MaxLength:=253
 	//+kubebuilder:validation:Pattern:=^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$


Should this also be CEL validation?

grokspawn requested a review from a team as a code owner October 22, 2024 19:12

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 22, 2024

grokspawn mentioned this pull request Oct 22, 2024

External API review submission #1358

Closed

everettraven reviewed Oct 22, 2024

View reviewed changes

grokspawn force-pushed the api-audit-updates branch from b7e2f01 to ae86221 Compare November 4, 2024 22:03

perdasilva reviewed Nov 5, 2024

View reviewed changes

updates from api audit

7a14b4d

Signed-off-by: Jordan Keister <jordan@nimblewidget.com>

grokspawn force-pushed the api-audit-updates branch from ae86221 to 7a14b4d Compare November 5, 2024 18:59

everettraven reviewed Nov 6, 2024

View reviewed changes

joelanford reviewed Nov 6, 2024

View reviewed changes

everettraven reviewed Nov 6, 2024

View reviewed changes

joelanford reviewed Nov 6, 2024

View reviewed changes

everettraven reviewed Nov 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: ⚠️ updates from api audit #1404

WIP: ⚠️ updates from api audit #1404

grokspawn commented Oct 22, 2024

netlify bot commented Oct 22, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024

everettraven Oct 22, 2024

grokspawn Nov 4, 2024

everettraven Oct 22, 2024

grokspawn Oct 23, 2024

everettraven Oct 24, 2024

perdasilva Nov 5, 2024

joelanford Nov 6, 2024

everettraven Nov 6, 2024

everettraven Nov 6, 2024

everettraven Nov 6, 2024

everettraven Nov 6, 2024

everettraven Nov 6, 2024

joelanford Nov 6, 2024

joelanford Nov 6, 2024

everettraven Nov 6, 2024

everettraven Nov 6, 2024

everettraven Nov 6, 2024

joelanford Nov 6, 2024

everettraven Nov 6, 2024

		@@ -80,14 +82,15 @@ const SourceTypeCatalog = "Catalog"
		type SourceConfig struct {

		@@ -130,6 +133,7 @@ type ClusterExtensionInstallConfig struct {
		//+kubebuilder:validation:Pattern:=^[a-z0-9]([-a-z0-9]*[a-z0-9])?$

		// This field is required when the spec.install.preflight.crdUpgradeSafety field is
		// specified.

		@@ -181,6 +186,7 @@ type CatalogSource struct {
		//+kubebuilder:validation:MaxLength:=253
		//+kubebuilder:validation:Pattern:=^[a-z0-9]([-a-z0-9][a-z0-9])?(\.[a-z0-9]([-a-z0-9][a-z0-9])?)*$

WIP: ⚠️ updates from api audit #1404

Are you sure you want to change the base?

WIP: ⚠️ updates from api audit #1404

Conversation

grokspawn commented Oct 22, 2024

Description

Reviewer Checklist

netlify bot commented Oct 22, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented Oct 22, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Oct 22, 2024 •

edited

Loading