✨ logically group fields in the spec #1200

yashoza19 · 2024-08-30T16:03:55Z

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-08-30T16:04:10Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`55d91d0`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/66d8be77d946f900084a8acc
😎 Deploy Preview	https://deploy-preview-1200--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.

api/v1alpha1/clusterextension_types.go

everettraven · 2024-08-30T17:13:40Z

internal/controllers/clusterextension_controller.go

+		ext.Status.Resolution.Bundle = nil
+		ext.Status.Install.Bundle = nil


Anywhere this change has been made runs a risk of accessing a nil pointer for the ext.Status.Resolution and ext.Status.Install fields. It looks like this is why some of the tests are failing (although maybe not caused by this line explicitly).

All areas where we update to this should perform some kind of nil check, and if the parent field is nil, set it. Something like:

Suggested change

ext.Status.Resolution.Bundle = nil

ext.Status.Install.Bundle = nil

if ext.Status.Resolution == nil {

ext.Status.Resolution = &ocv1alpha1.ResolutionStatus{}

}

ext.Status.Resolution.Bundle = nil

if ext.Status.Install == nil {

ext.Status.Install = &ocv1alpha1.InstallStatus{}

}

ext.Status.Install.Bundle = nil

Since you'd have to likely do this nil check on every place we set this, it might be helpful to extract it into a helper function for each status that looks something like:

func setResolutionStatus(ext *ocv1alpha1.ClusterExtension, resStatus *ocv1alpha1.ResolutionStatus) { ... } func setInstallStatus(ext *ocv1alpha1.ClusterExtension, installStatus *ocv1alpha1.InstallStatus) { ... }

api/v1alpha1/clusterextension_types.go

everettraven · 2024-09-03T13:48:32Z

hack/test/pre-upgrade-setup.sh

+  namespace: default
  serviceAccount:


Indentation required?

internal/controllers/clusterextension_controller.go

everettraven · 2024-09-03T13:58:51Z

internal/controllers/clusterextension_controller.go

+	resolvedBundleMetadata := bundleutil.MetadataFor(resolvedBundle.Name, *resolvedBundleVersion)
+	resStatus := &ocv1alpha1.ResolutionStatus{
+		Bundle: &ocv1alpha1.BundleMetadata{
+			Name:    resolvedBundleMetadata.Name,
+			Version: resolvedBundleMetadata.Version,
+		},
+	}


Suggested change

resolvedBundleMetadata := bundleutil.MetadataFor(resolvedBundle.Name, *resolvedBundleVersion)

resStatus := &ocv1alpha1.ResolutionStatus{

Bundle: &ocv1alpha1.BundleMetadata{

Name: resolvedBundleMetadata.Name,

Version: resolvedBundleMetadata.Version,

},

}

resStatus := &ocv1alpha1.ResolutionStatus{

Bundle: bundleutil.MetadataFor(resolvedBundle.Name, *resolvedBundleVersion),

}

everettraven · 2024-09-03T13:59:45Z

internal/controllers/clusterextension_controller.go

+	installedBundleMetadata := bundleutil.MetadataFor(resolvedBundle.Name, *resolvedBundleVersion)
+	installStatus := &ocv1alpha1.InstallStatus{
+		Bundle: &ocv1alpha1.BundleMetadata{
+			Name:    installedBundleMetadata.Name,
+			Version: installedBundleMetadata.Version,
+		},
+	}


Suggested change

installedBundleMetadata := bundleutil.MetadataFor(resolvedBundle.Name, *resolvedBundleVersion)

installStatus := &ocv1alpha1.InstallStatus{

Bundle: &ocv1alpha1.BundleMetadata{

Name: installedBundleMetadata.Name,

Version: installedBundleMetadata.Version,

},

}

installStatus := &ocv1alpha1.InstallStatus{

Bundle: bundleutil.MetadataFor(resolvedBundle.Name, *resolvedBundleVersion),

}

everettraven · 2024-09-03T14:00:21Z

internal/controllers/common_controller.go

+	if ext.Status.Resolution == nil {
+		ext.Status.Resolution = &ocv1alpha1.ResolutionStatus{}
+	}
+	ext.Status.Resolution.Bundle = resStatus.Bundle


Suggested change

if ext.Status.Resolution == nil {

ext.Status.Resolution = &ocv1alpha1.ResolutionStatus{}

}

ext.Status.Resolution.Bundle = resStatus.Bundle

ext.Status.Resolution = resStatus

everettraven · 2024-09-03T14:00:43Z

internal/controllers/common_controller.go

+	if ext.Status.Install == nil {
+		ext.Status.Install = &ocv1alpha1.InstallStatus{}
+	}
+	ext.Status.Install.Bundle = installStatus.Bundle


Suggested change

if ext.Status.Install == nil {

ext.Status.Install = &ocv1alpha1.InstallStatus{}

}

ext.Status.Install.Bundle = installStatus.Bundle

ext.Status.Install = installStatus

everettraven · 2024-09-04T15:29:20Z

test/e2e/cluster_extension_install_test.go

@@ -319,7 +326,7 @@ func TestClusterExtensionInstallRegistryMultipleBundles(t *testing.T) {
 		assert.Equal(ct, metav1.ConditionFalse, cond.Status)
 		assert.Equal(ct, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
 		assert.Contains(ct, cond.Message, "matching packages found in multiple catalogs")
-		assert.Nil(ct, clusterExtension.Status.ResolvedBundle)
+		assert.Nil(ct, clusterExtension.Status.Resolution.Bundle)


This is probably where the e2e panic is

Suggested change

assert.Nil(ct, clusterExtension.Status.Resolution.Bundle)

assert.Nil(ct, clusterExtension.Status.Resolution)

everettraven

Overall it looks really good! Fix the e2e panic and I'll be happy to merge!

joelanford · 2024-09-04T18:05:55Z

api/v1alpha1/clusterextension_types.go

+	//    namespace: example-namespace
+	//    serviceAccount:
+	//      name: example-sa
+	Install InstallationConfig `json:"install"`


Let's try to be consistent with word forms between field names and type names:

Suggested change

Install InstallationConfig `json:"install"`

Install InstallConfig `json:"install"`

Also, should we be prefixing these type names with ClusterExtension?

This package will contain structs for any future root API types and their sub-structs.

A hypothetical Extension API might have its own InstallConfig struct, but it will be different because it would not have a namespace field.

Do you mean it should be ClusterExtensionInstallConfig?

I think ClusterExtensionInstallConfig is reasonable

But I think @joelanford is referring to all the new types here. So something like:

ClusterExtensionInstallConfig

ClusterExtensionInstallStatus

ClusterExtensionResolutionStatus

...

codecov · 2024-09-04T18:38:46Z

Codecov Report

Attention: Patch coverage is 60.27397% with 29 lines in your changes missing coverage. Please review.

Project coverage is 77.07%. Comparing base (f797d50) to head (55d91d0).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
api/v1alpha1/zz_generated.deepcopy.go	52.17%	21 Missing and 1 partial ⚠️
internal/applier/helm.go	42.85%	3 Missing and 1 partial ⚠️
...nternal/controllers/clusterextension_controller.go	75.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1200      +/-   ##
==========================================
- Coverage   77.56%   77.07%   -0.49%     
==========================================
  Files          36       36              
  Lines        1979     2020      +41     
==========================================
+ Hits         1535     1557      +22     
- Misses        309      328      +19     
  Partials      135      135

Flag	Coverage Δ
e2e	`57.62% <57.53%> (-0.04%)`	⬇️
unit	`55.34% <19.17%> (-0.80%)`	⬇️

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.

joelanford · 2024-09-04T18:51:35Z

test/e2e/cluster_extension_install_test.go

+				Name:    "prometheus-operator.1.2.0",
+				Version: "1.2.0",


This is not a change I expect to see in a PR that is rearranging types. What's the background here?

In order to compare the BundleMetadata to the new Resolution.Bundle spec, I had to make these changes.

To clarify, since these are in an Eventually block, the e2e tests were panicking due to accessing the nil clusterExtension.Status.Resolution object to get the Bundle field. Doing this compares the entire clusterExtension.Status.Resolution object and avoids a potential panic. Alternatively an explicit nil check could be done before attempting to access the Bundle field.

I should have been more specific in this comment (and it's no big deal since @tmshort fixed it in #1222), but what I was trying to ask is why this changed from 2.0.0 to 1.2.0.

I'm guessing that the Eventually aspect of this test is what tripped us up here. It is likely the case that the value is 1.2.0 when this check executes the first time, which explains why this would pass. However the purpose of this test is to see if a new catalog showing up under the same tag will trigger a re-reconcile and resolve a new version.

By setting this to 1.2.0, we were not actually testing what this test intended. Again, no big deal and the actual code was still behaving as expected, which is how Todd caught it.

However, I wonder: can we design our tests to be more robust against this kind of thing? Use of Eventually is likely the primary source of this problem because it implies both of:

the current state of the field is not what we expect (otherwise, we would just directly check it)

the value of the field we are asserting against will change over time.

And that means that the test can pass using any assertion that passes on any initial or transitory state of that field.

Signed-off-by: yashoza19 <[email protected]>

everettraven

Looks good to me! Thanks @yashoza19 !

yashoza19 requested a review from a team as a code owner August 30, 2024 16:03

everettraven requested changes Aug 30, 2024

View reviewed changes

api/v1alpha1/clusterextension_types.go Outdated Show resolved Hide resolved

api/v1alpha1/clusterextension_types.go Outdated Show resolved Hide resolved

yashoza19 force-pushed the issue1148 branch 2 times, most recently from ba4c86b to 2019ab1 Compare August 30, 2024 17:00

everettraven reviewed Aug 30, 2024

View reviewed changes

yashoza19 force-pushed the issue1148 branch 2 times, most recently from d3a7526 to 384ee03 Compare September 3, 2024 13:57

everettraven requested changes Sep 3, 2024

View reviewed changes

yashoza19 force-pushed the issue1148 branch 2 times, most recently from 7b6a04b to 8e5835a Compare September 4, 2024 13:57

everettraven reviewed Sep 4, 2024

View reviewed changes

yashoza19 force-pushed the issue1148 branch from 8e5835a to 8800c06 Compare September 4, 2024 16:48

joelanford reviewed Sep 4, 2024

View reviewed changes

yashoza19 force-pushed the issue1148 branch 2 times, most recently from 164da28 to cc8c5f2 Compare September 4, 2024 18:28

joelanford reviewed Sep 4, 2024

View reviewed changes

joelanford mentioned this pull request Sep 4, 2024

📖 [Docs] Add support for skip ranges and replacements #1215

Merged

4 tasks

yashoza19 force-pushed the issue1148 branch from cc8c5f2 to 9949826 Compare September 4, 2024 19:39

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

yashoza19 force-pushed the issue1148 branch from 9949826 to 32a86c2 Compare September 4, 2024 20:02

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 4, 2024

logically group fields in the spec

55d91d0

Signed-off-by: yashoza19 <[email protected]>

yashoza19 force-pushed the issue1148 branch from 32a86c2 to 55d91d0 Compare September 4, 2024 20:09

everettraven approved these changes Sep 4, 2024

View reviewed changes

everettraven added this pull request to the merge queue Sep 4, 2024

Merged via the queue into operator-framework:main with commit 557e28f Sep 4, 2024
15 of 19 checks passed

This was referenced Sep 5, 2024

📖[Docs] How-to: Z-stream Automatic Updates #1219

Merged

📖[Docs] How-to: Pin Version and Disable Automatic Updates #1221

Merged

📖[Docs] How-to: Channel-Based Automatic Updates #1220

Merged

skattoju mentioned this pull request Sep 9, 2024

🌱 Descriptive Upgrade Constraint Policy constants #1233

Merged

skattoju mentioned this pull request Sep 25, 2024

✨ Remove unpacked status #1309

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ logically group fields in the spec #1200

✨ logically group fields in the spec #1200

yashoza19 commented Aug 30, 2024

netlify bot commented Aug 30, 2024 •

edited

Loading

everettraven Aug 30, 2024

everettraven Sep 3, 2024

everettraven Sep 3, 2024

everettraven Sep 3, 2024

everettraven Sep 3, 2024

everettraven Sep 3, 2024

everettraven Sep 4, 2024

everettraven left a comment

joelanford Sep 4, 2024

joelanford Sep 4, 2024

yashoza19 Sep 4, 2024

everettraven Sep 4, 2024

everettraven Sep 4, 2024

codecov bot commented Sep 4, 2024 •

edited

Loading

joelanford Sep 4, 2024

yashoza19 Sep 4, 2024

everettraven Sep 4, 2024

joelanford Sep 9, 2024

everettraven left a comment

		ext.Status.Resolution.Bundle = nil
		ext.Status.Install.Bundle = nil

	assert.Nil(ct, clusterExtension.Status.Resolution.Bundle)
	assert.Nil(ct, clusterExtension.Status.Resolution)

	Install InstallationConfig `json:"install"`
	Install InstallConfig `json:"install"`

✨ logically group fields in the spec #1200

✨ logically group fields in the spec #1200

Conversation

yashoza19 commented Aug 30, 2024

Description

Reviewer Checklist

netlify bot commented Aug 30, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

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

everettraven left a 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

codecov bot commented Sep 4, 2024 • edited Loading

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

everettraven left a comment

Choose a reason for hiding this comment

netlify bot commented Aug 30, 2024 •

edited

Loading

codecov bot commented Sep 4, 2024 •

edited

Loading