⚠️ updates from api audit by grokspawn · Pull Request #1404 · operator-framework/operator-controller

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

This is API changes in preparation for our v1.0 release, mostly from the openshift API team's audit but also from feedback from the maintainers and community.

It is expected that this PR contains breaking changes as we prepare (tightening types, enhancing docs, etc.) for the release.

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	`1620b73`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/672e88cdbf71a5000878c4c3
😎 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

Attention: Patch coverage is 71.79487% with 11 lines in your changes missing coverage. Please review.

Project coverage is 74.88%. Comparing base (ce86a65) to head (1620b73).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/applier/helm.go	35.29%	9 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #1404 +/- ##
==========================================
- Coverage 75.18% 74.88% -0.31% 
==========================================
Files 42 42 Lines 3236 3237 +1 ==========================================
- Hits 2433 2424 -9 - Misses 632 640 +8 - Partials 171 173 +2

Flag	Coverage Δ
e2e	`51.93% <64.10%> (-0.36%)`	⬇️
unit	`57.24% <30.76%> (-0.42%)`	⬇️

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.

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

 	//
 	//+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?

What do you have in mind? I was specifically modeling after https://github.com/openshift/api/blob/4f6053f954b069f7fea5788d4307f4213db53678/config/v1alpha1/types_image_policy.go#L56-L60

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

+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.

I resolved this by externalizing the conditions somewhat, but still putting all the 'CRDUpgradeSafety' checks in one place.
My assertion is still that this kind of thing should be exposed via an interface.

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

LalatenduMohanty

/lgtm

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

openshift-ci · 2024-11-08T21:55:26Z

New changes are detected. LGTM label has been removed.

grokspawn · 2024-11-11T20:59:15Z

Solves #1426

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

openshift-ciBot 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

Comment threadapi/v1alpha1/clusterextension_types.go Outdated

everettraven reviewed Oct 22, 2024
View reviewed changes

Comment threadconfig/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml Outdated

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

perdasilva reviewed Nov 5, 2024
View reviewed changes

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

everettraven reviewed Nov 6, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go

everettraven reviewed Nov 6, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go Outdated

everettraven reviewed Nov 6, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go

everettraven reviewed Nov 6, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go Outdated

joelanford reviewed Nov 6, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go

joelanford reviewed Nov 6, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go Outdated

everettraven reviewed Nov 6, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go Outdated

everettraven reviewed Nov 6, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go Outdated

everettraven reviewed Nov 6, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go

joelanford reviewed Nov 6, 2024
View reviewed changes

everettraven reviewed Nov 6, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go Outdated

everettraven reviewed Nov 6, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go

grokspawn force-pushed the api-audit-updates branch 6 times, most recently from f04b254 to 237bcf6Compare November 7, 2024 19:54

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2024

grokspawn force-pushed the api-audit-updates branch from 237bcf6 to a3684a6Compare November 7, 2024 20:28

everettraven reviewed Nov 7, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go Outdated

everettraven reviewed Nov 7, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go Outdated

everettraven reviewed Nov 7, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go

LalatenduMohanty reviewed Nov 7, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go

everettraven reviewed Nov 7, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go

LalatenduMohanty reviewed Nov 7, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go

everettraven reviewed Nov 7, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go Outdated

everettraven reviewed Nov 7, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go Outdated

everettraven reviewed Nov 7, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go Outdated

everettraven reviewed Nov 7, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go Outdated

everettraven reviewed Nov 8, 2024
View reviewed changes

Comment threadapi/v1alpha1/clusterextension_types.go

everettraven reviewed Nov 8, 2024
View reviewed changes

Comment threadinternal/applier/helm.go Outdated

grokspawn force-pushed the api-audit-updates branch from 18552d0 to 22ef06fCompare November 8, 2024 16:46

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

grokspawn force-pushed the api-audit-updates branch from 22ef06f to 8189015Compare November 8, 2024 19:48

LalatenduMohanty previously approved these changes Nov 8, 2024
View reviewed changes

openshift-ciBot assigned LalatenduMohanty Nov 8, 2024

openshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2024

everettraven mentioned this pull request Nov 8, 2024
⚠️ Move .spec.install.namespace and .spec.install.serviceAccount to .spec.namespace and .spec.serviceAccount#1439
Merged
4 tasks

e2e fixes
1620b73
Signed-off-by: Jordan Keister <jordan@nimblewidget.com>

grokspawn dismissed LalatenduMohanty’s stale review via 1620b73November 8, 2024 21:55

openshift-ciBot removed the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2024

everettraven approved these changes Nov 8, 2024
View reviewed changes

everettraven enabled auto-merge November 8, 2024 21:57

everettraven added this pull request to the merge queue Nov 8, 2024

Merged via the queue into operator-framework:main with commit 6c2be08Nov 8, 2024

grokspawn deleted the api-audit-updates branch November 8, 2024 22:13

Uh oh!

Conversation

grokspawn commented Oct 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Checklist

Uh oh!

netlifyBot commented Oct 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

codecovBot commented Oct 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

perdasilvaNov 5, 2024

Choose a reason for hiding this comment

Uh oh!

joelanfordNov 6, 2024

Choose a reason for hiding this comment

Uh oh!

everettravenNov 6, 2024

Choose a reason for hiding this comment

Uh oh!

grokspawnNov 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joelanfordNov 6, 2024

Choose a reason for hiding this comment

Uh oh!

grokspawnNov 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LalatenduMohanty left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ciBot commented Nov 8, 2024

Uh oh!

grokspawn commented Nov 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

grokspawn commented Oct 22, 2024•
edited
Loading

netlifyBot commented Oct 22, 2024•
edited
Loading

codecovBot commented Oct 22, 2024•
edited
Loading