⚠ REMOVE extension api from main branch #820

acmenezes · 2024-05-02T19:04:10Z

Description

The intent is to remove this API and its controllers for the v1.0.0 release. Once we've released 1.0.0, we plan to re-introduce the Extension API and controllers.

@joelanford, please take a look and tell me if that's what you had in mind. I did a flat removal of all the references to the api as well. Didn't try to circumvent or redesign around any errors. Any suggestions or changes are welcomed. Saving this as a draft PR to be safe.

My question, and I think it's an important one, would we pull the main branch's code to some secondary branch as a reference for after v1.0.0 before merging this if we plan to reintroduce it?

Solves #735

Reviewer Checklist

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

netlify · 2024-05-02T19:04:25Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`5f83813`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/66437b470d70e70008b3e7a8
😎 Deploy Preview	https://deploy-preview-820--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-05-02T19:10:48Z

Codecov Report

Attention: Patch coverage is 66.66667% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 81.80%. Comparing base (6d73b73) to head (ca00f4a).

Files	Patch %	Lines
...nternal/controllers/clusterextension_controller.go	57.14%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #820       +/-   ##
===========================================
+ Coverage   71.02%   81.80%   +10.78%     
===========================================
  Files          17       15        -2     
  Lines        1301      907      -394     
===========================================
- Hits          924      742      -182     
+ Misses        304      120      -184     
+ Partials       73       45       -28

Flag	Coverage Δ
e2e	`59.75% <61.11%> (+17.02%)`	⬆️
unit	`73.76% <66.66%> (+10.16%)`	⬆️

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-05-03T18:03:48Z

I don't think we need to save the code off. The git history is there, so we can just come back to the commit prior to this PR if necessary.

But I suspect we don't even do that. I think it is more likely that we'll essentially copy ClusterExtension back over to Extension and make the few tweaks necessary for it to work as a namespace-scoped API. And we'll do a similar thing for the controller.

joelanford · 2024-05-03T18:04:30Z

api/v1alpha1/clusterextension_types.go

-	// +optional
-	InstalledBundle *BundleMetadata `json:"installedBundle,omitempty"`
-	// +optional
-	ResolvedBundle *BundleMetadata `json:"resolvedBundle,omitempty"`
-


These should stay. If BundleMetadata is defined in extension_types.go, let's move those definitions to this file.

joelanford · 2024-05-03T18:05:46Z

config/rbac/role.yaml

- apiGroups:
-  - kappctrl.k14s.io


We should remove the kapp dependency (I think you'll see breadcrumbs in Makefile and the install script template).

Cool. Thanks I will check on that one yet.

Hi @joelanford , I finally removed kapp from Makefile, install scripts and also from the scheme package I put on the last PR. I guess that should cover it. Let me know.

joelanford · 2024-05-03T18:06:51Z

internal/controllers/clusterextension_controller.go

@@ -121,17 +121,14 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 	// Lookup the bundle that corresponds to the ClusterExtension's desired package.
 	bundle, err := r.resolve(ctx, ext)
 	if err != nil {
-		ext.Status.InstalledBundle = nil


Re: keeping these fields in ClusterExtension, we also want to keep the ClusterExtension reconciler logic intact.

Ok. Ack. Keeping it intact.

internal/controllers/common_controller.go

everettraven · 2024-05-07T18:32:02Z

api/v1alpha1/clusterextension_types.go

@@ -106,6 +115,7 @@ func init() {
 		TypePackageDeprecated,
 		TypeChannelDeprecated,
 		TypeBundleDeprecated,
+		TypeProgressing,


Unit tests are probably failing due to the Progressing type being added to the set of conditions but it never being set on the ClusterExtension during the reconciliation process.

I would keep the type definitions for the condition and reasons but would remove them from the list here. IMO to keep the scope of this PR small, the implementation/usage of them should take place in #747

Hi @everettraven , actually it will fail another test and ask me to add it to the list. What I did was adding that to the list but then it requires me to actually set progressing status in the ClusterExtension controller. That part I'm honestly not sure if I did right. I appreciate some eyes on this. On the other hand if you really want to keep the type out of the list of conditions sets then we may have to change some tests. Happy to go either way. I count on your directions here.

Oh that is interesting. I didn't realize there was a test that we had the ensured those constants are added to the conditionsets.

I'm not sure this is really a valuable test for us at the moment. I'm not sure what the best path forward here is personally, maybe:

Merge ✨ remove extension-specific type checks #842

Rebase this PR on ^, so it essentially becomes shifting the logic around but the clusterextension_controller already uses everything so we don't have to fight with CI

Makes sense. I think we can talk about it on our meeting on Tuesday.

actually it will fail another test and ask me to add it to the list.

What was the specific test failure? I think @everettraven is correct that we should be able to let this status condition type be completely removed since it is currently only present in the Extension API.

@joelanford I think it is these test cases:

operator-controller/api/v1alpha1/clusterextension_types_test.go

Lines 18 to 63 in e9acbd3

func TestClusterExtensionTypeRegistration(t *testing.T) {

types, err := parseConstants("Type")

if err != nil {

t.Fatalf("unable to parse Type constants %v", err)

}

for _, tt := range types {

if !slices.Contains(conditionsets.ConditionTypes, tt) && !slices.Contains(conditionsets.ExtensionConditionTypes, tt) {

t.Errorf("append Type%s to conditionsets.ConditionTypes in this package's init function", tt)

}

}

for _, tt := range conditionsets.ConditionTypes {

if !slices.Contains(types, tt) {

t.Errorf("there must be a Type%[1]s string literal constant for type %[1]q (i.e. 'const Type%[1]s = %[1]q')", tt)

}

}

for _, tt := range conditionsets.ExtensionConditionTypes {

if !slices.Contains(types, tt) {

t.Errorf("there must be a Type%[1]s string literal constant for type %[1]q (i.e. 'const Type%[1]s = %[1]q')", tt)

}

}

}

func TestClusterExtensionReasonRegistration(t *testing.T) {

reasons, err := parseConstants("Reason")

if err != nil {

t.Fatalf("unable to parse Reason constants %v", err)

}

for _, r := range reasons {

if !slices.Contains(conditionsets.ConditionReasons, r) && !slices.Contains(conditionsets.ExtensionConditionReasons, r) {

t.Errorf("append Reason%s to conditionsets.ConditionReasons in this package's init function.", r)

}

}

for _, r := range conditionsets.ConditionReasons {

if !slices.Contains(reasons, r) {

t.Errorf("there must be a Reason%[1]s string literal constant for reason %[1]q (i.e. 'const Reason%[1]s = %[1]q')", r)

}

}

for _, r := range conditionsets.ExtensionConditionReasons {

if !slices.Contains(reasons, r) {

t.Errorf("there must be a Reason%[1]s string literal constant for reason %[1]q (i.e. 'const Reason%[1]s = %[1]q')", r)

}

}

}

everettraven

IMO the lint failures can be addressed via a //nolint:unused comment for now and be updated in #747 when they are used again.

This should help keep the scope of this PR smaller

acmenezes · 2024-05-07T20:34:20Z

IMO the lint failures can be addressed via a //nolint:unused comment for now and be updated in #747 when they are used again.

This should help keep the scope of this PR smaller

@everettraven good idea. I just added the comments there. I think no more lint issues are found. Thanks.

varshaprasad96

@acmenezes There is also a Kapp CRD in testdata: https://github.com/operator-framework/operator-controller/blob/b82812ea96a44eccc4ac4bd9bbfe8b023271130d/testdata/crds/kappctrl.k14s.io_app.yaml, could you also remove this please? Thanks!

varshaprasad96 · 2024-05-13T21:42:12Z

api/v1alpha1/clusterextension_types.go

+	ReasonFailedToReachDesiredIntent = "FailedToReachDesiredIntent"
+	ReasonReachedDesiredIntent       = "ReachedDesiredIntent"


IIRC these were related to upgrades and being used in extension-controller. These can be removed too.

everettraven

LGTM! Great work @acmenezes !

varshaprasad96

/lgtm
/approve

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 May 2, 2024

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

joelanford reviewed May 3, 2024

View reviewed changes

internal/controllers/common_controller.go Show resolved Hide resolved

acmenezes force-pushed the remove_extension branch from f45e655 to 02f0f7c Compare May 3, 2024 20:38

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

acmenezes force-pushed the remove_extension branch from 02f0f7c to cc18928 Compare May 6, 2024 20:41

everettraven reviewed May 7, 2024

View reviewed changes

acmenezes force-pushed the remove_extension branch 6 times, most recently from ca00f4a to b82812e Compare May 7, 2024 20:33

everettraven mentioned this pull request May 10, 2024

✨ remove extension-specific type checks #842

Merged

varshaprasad96 requested changes May 13, 2024

View reviewed changes

acmenezes force-pushed the remove_extension branch from b82812e to 2e30548 Compare May 14, 2024 14:48

REMOVE extension api from main branch

5f83813

acmenezes force-pushed the remove_extension branch from 2e30548 to 5f83813 Compare May 14, 2024 14:55

acmenezes marked this pull request as ready for review May 14, 2024 15:24

acmenezes requested a review from a team as a code owner May 14, 2024 15:24

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

everettraven approved these changes May 14, 2024

View reviewed changes

everettraven requested a review from varshaprasad96 May 14, 2024 16:59

varshaprasad96 approved these changes May 14, 2024

View reviewed changes

openshift-ci bot assigned varshaprasad96 May 14, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 14, 2024

everettraven added this pull request to the merge queue May 14, 2024

Merged via the queue into operator-framework:main with commit a458282 May 14, 2024
14 of 15 checks passed

acmenezes mentioned this pull request Jun 5, 2024

🐛REMOVE extension remaining CRD #911

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠ REMOVE extension api from main branch #820

⚠ REMOVE extension api from main branch #820

acmenezes commented May 2, 2024

netlify bot commented May 2, 2024 •

edited

Loading

codecov bot commented May 2, 2024 •

edited

Loading

joelanford commented May 3, 2024

joelanford May 3, 2024

acmenezes May 3, 2024

joelanford May 3, 2024

acmenezes May 3, 2024

acmenezes May 7, 2024

joelanford May 3, 2024 •

edited

Loading

acmenezes May 3, 2024

everettraven May 7, 2024

acmenezes May 7, 2024

everettraven May 10, 2024

acmenezes May 10, 2024

joelanford May 13, 2024

everettraven May 13, 2024

everettraven left a comment

acmenezes commented May 7, 2024

varshaprasad96 left a comment

varshaprasad96 May 13, 2024

everettraven left a comment

varshaprasad96 left a comment

	func TestClusterExtensionTypeRegistration(t *testing.T) {
	types, err := parseConstants("Type")
	if err != nil {
	t.Fatalf("unable to parse Type constants %v", err)
	}

	for _, tt := range types {
	if !slices.Contains(conditionsets.ConditionTypes, tt) && !slices.Contains(conditionsets.ExtensionConditionTypes, tt) {
	t.Errorf("append Type%s to conditionsets.ConditionTypes in this package's init function", tt)
	}
	}

	for _, tt := range conditionsets.ConditionTypes {
	if !slices.Contains(types, tt) {
	t.Errorf("there must be a Type%[1]s string literal constant for type %[1]q (i.e. 'const Type%[1]s = %[1]q')", tt)
	}
	}
	for _, tt := range conditionsets.ExtensionConditionTypes {
	if !slices.Contains(types, tt) {
	t.Errorf("there must be a Type%[1]s string literal constant for type %[1]q (i.e. 'const Type%[1]s = %[1]q')", tt)
	}
	}
	}

	func TestClusterExtensionReasonRegistration(t *testing.T) {
	reasons, err := parseConstants("Reason")
	if err != nil {
	t.Fatalf("unable to parse Reason constants %v", err)
	}

	for _, r := range reasons {
	if !slices.Contains(conditionsets.ConditionReasons, r) && !slices.Contains(conditionsets.ExtensionConditionReasons, r) {
	t.Errorf("append Reason%s to conditionsets.ConditionReasons in this package's init function.", r)
	}
	}
	for _, r := range conditionsets.ConditionReasons {
	if !slices.Contains(reasons, r) {
	t.Errorf("there must be a Reason%[1]s string literal constant for reason %[1]q (i.e. 'const Reason%[1]s = %[1]q')", r)
	}
	}
	for _, r := range conditionsets.ExtensionConditionReasons {
	if !slices.Contains(reasons, r) {
	t.Errorf("there must be a Reason%[1]s string literal constant for reason %[1]q (i.e. 'const Reason%[1]s = %[1]q')", r)
	}
	}
	}

		ReasonFailedToReachDesiredIntent = "FailedToReachDesiredIntent"
		ReasonReachedDesiredIntent = "ReachedDesiredIntent"

⚠ REMOVE extension api from main branch #820

⚠ REMOVE extension api from main branch #820

Conversation

acmenezes commented May 2, 2024

Description

Reviewer Checklist

netlify bot commented May 2, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented May 2, 2024 • edited Loading

Codecov Report

joelanford commented May 3, 2024

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

joelanford May 3, 2024 • edited Loading

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

everettraven left a comment

Choose a reason for hiding this comment

acmenezes commented May 7, 2024

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

varshaprasad96 left a comment

Choose a reason for hiding this comment

netlify bot commented May 2, 2024 •

edited

Loading

codecov bot commented May 2, 2024 •

edited

Loading

joelanford May 3, 2024 •

edited

Loading