feat: Update Operator SDK version strategy #1250

mateusoliveira43 · 2023-12-01T20:40:53Z

The goal of this PR is to create a strategy (document or script) for updating Operator SDK version in the project.

My goal was to have all operator generated files in a separate folder (inspired by dora-metrics/pelorus#1045), but helm and Go Operator SDK structures are different, its not the best idea to try to put Go generated files in a separate folder.

The idea is: in temporaries folders, create Operator SDK structure using the current Operator SDK version and the version which we want to upgrade it to, following these steps:
operator-sdk init --project-name=oadp-operator --repo=github.com/openshift/oadp-operator --domain=openshift.io
operator-sdk create api --group oadp --version v1alpha1 --kind DataProtectionApplication --resource --controller
operator-sdk create api --group oadp --version v1alpha1 --kind CloudStorage --resource --controller

Then generate a diff file and apply changes to repo code.
It seems to work operator-framework/operator-sdk#6628

There are still a few points to adjust, like: when do we update boilerplate year? What to update in go.mod? Create another Makefile? ( the example added can be run with make -f oadp.mk help)

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

openshift-ci · 2023-12-01T20:40:58Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2023-12-01T20:41:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mateusoliveira43
Once this PR has been reviewed and has the lgtm label, please assign jmontleon for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mateusoliveira43 · 2023-12-01T20:44:33Z

op.diff

example of diff file between current and next operator sdk version structures

mateusoliveira43 · 2023-12-04T11:56:29Z

Should we update hack/boilerplate.go.txt to match this?

oadp-operator/LICENSE

Lines 179 to 191 in 0347669

    
           Copyright 2016 Red Hat, Inc. 
        
           Licensed under the Apache License, Version 2.0 (the "License"); 
        
           you may not use this file except in compliance with the License. 
        
           You may obtain a copy of the License at 
        
               http://www.apache.org/licenses/LICENSE-2.0 
        
           Unless required by applicable law or agreed to in writing, software 
        
           distributed under the License is distributed on an "AS IS" BASIS, 
        
           WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. 
        
           See the License for the specific language governing permissions and 
        
           limitations under the License.

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

mateusoliveira43 · 2023-12-04T12:47:39Z

.dockerignore

Instead of creating new Makefile, we could just add our commands to end of the file, like this. This should avoid patching problems

mateusoliveira43 · 2023-12-04T12:49:20Z

controllers/predicate.go

@@ -40,7 +40,7 @@ func isObjectOurs(scheme *runtime.Scheme, object client.Object) bool {
 		return false
 	}
 	gvk := objGVKs[0]
-	if gvk.Group == oadpv1alpha1.GroupVersion.Group && gvk.Version == oadpv1alpha1.GroupVersion.Version && gvk.Kind == oadpv1alpha1.Kind {


this change is ok or we only care about DPA objects here?

mateusoliveira43 · 2023-12-04T12:58:42Z

For every file accepted to change name, run grep command to check doc occurrences and change them as well

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

mateusoliveira43 · 2023-12-05T12:02:00Z

controllers/cloudstorage_controller.go

 	Client        client.Client
 	Scheme        *runtime.Scheme
 	Log           logr.Logger
 	EventRecorder record.EventRecorder
 }

-//TODO!!! FIX THIS!!!!
+//TODO!!! FIX THIS!!!!?
+// TODO change buckets to cloudstorages?


this and leases role need updating

mateusoliveira43 · 2023-12-05T18:30:50Z

also run operator-sdk create webhook command after operator-sdk create api?

kaovilai · 2023-12-06T03:53:42Z

api/v1alpha1/groupversion_info.go

@@ -28,8 +28,6 @@ var (
 	// GroupVersion is group version used to register these objects
 	GroupVersion = schema.GroupVersion{Group: "oadp.openshift.io", Version: "v1alpha1"}

-	Kind = "DataProtectionApplication"


Do we wanna add kind back and add to schemebuilder line 32?

I think not, I believe this was used only as a constant here

oadp-operator/controllers/predicate.go

Line 43 in 0347669

if gvk.Group == oadpv1alpha1.GroupVersion.Group && gvk.Version == oadpv1alpha1.GroupVersion.Version && gvk.Kind == oadpv1alpha1.Kind {

I believe the kind is get from theses structs name https://github.com/openshift/oadp-operator/blob/master/api/v1alpha1/oadp_types.go#L281, in each file

bundle/manifests/oadp-operator.clusterserviceversion.yaml

mpryc · 2023-12-06T15:31:53Z

The idea is really nice, may be tricky to maintain with more diffs.

We have something like that in other project, right?
https://github.com/dora-metrics/pelorus/tree/master/scripts/pelorus-operator-patches

The benefit is that we can always re-generate operator starting from scratch.

This is fine for operator where there is not a huge number of changes to the initial files generated by the operator-sdk however in the case of oadp-operator where majority of the code is implemented within that generated initial files this may be very hard to maintain, especially when we are talking later about backports to older releases.

What about using kustomize files instead of patches whenever applicable ?

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

mateusoliveira43 · 2023-12-06T15:53:13Z

@mpryc similar yes, but we will not have patch files here. Its terrible developer experience (editing a diff file instead of a yaml file, for example).

We would re-generate files only when we want to update opeartor-sdk version, then we would create a diff file (not tracked by git), and then apply it and then open a PR for that action.

kustomize files can be an option, but not for all things I think. Makefile may be updated, for example. And that is the thing I think we need to be more careful: editing generated files. So, when we update then, it is a smooth experience. Maybe just adding new things to the end of those files can be enough to avoid patching errors

openshift-ci · 2024-02-10T01:41:21Z

@mateusoliveira43: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-merge-robot · 2024-02-10T01:41:29Z

PR needs rebase.

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.

openshift-bot · 2024-05-10T09:01:00Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

mateusoliveira43 · 2024-05-10T14:33:22Z

/remove-lifecycle stale
Still want to work on this

feat: Update Operator SDK version strategy

2018a9d

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

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 Dec 1, 2023

mateusoliveira43 commented Dec 1, 2023

View reviewed changes

op.diff Outdated

Copy link

Contributor Author

mateusoliveira43 Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

example of diff file between current and next operator sdk version structures

fixup! feat: Update Operator SDK version strategy

69c34c6

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

mateusoliveira43 commented Dec 4, 2023

View reviewed changes

fixup! feat: Update Operator SDK version strategy

b25bbde

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

mateusoliveira43 commented Dec 5, 2023

View reviewed changes

kaovilai reviewed Dec 6, 2023

View reviewed changes

bundle/manifests/oadp-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved

fixup! feat: Update Operator SDK version strategy

756db8e

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

mateusoliveira43 mentioned this pull request Dec 11, 2023

feat: Add golangci-lint #1254

Closed

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

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 10, 2024

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 10, 2024

mateusoliveira43 mentioned this pull request May 29, 2024

OADP-4137: updated for velero 1.14 #1398

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update Operator SDK version strategy #1250

feat: Update Operator SDK version strategy #1250

mateusoliveira43 commented Dec 1, 2023

openshift-ci bot commented Dec 1, 2023

openshift-ci bot commented Dec 1, 2023

mateusoliveira43 Dec 1, 2023

mateusoliveira43 commented Dec 4, 2023

mateusoliveira43 Dec 4, 2023

mateusoliveira43 Dec 4, 2023

mateusoliveira43 commented Dec 4, 2023

mateusoliveira43 Dec 5, 2023 •

edited

mateusoliveira43 commented Dec 5, 2023

kaovilai Dec 6, 2023

mateusoliveira43 Dec 6, 2023

mpryc commented Dec 6, 2023

mateusoliveira43 commented Dec 6, 2023

openshift-ci bot commented Feb 10, 2024

openshift-merge-robot commented Feb 10, 2024

openshift-bot commented May 10, 2024

mateusoliveira43 commented May 10, 2024

feat: Update Operator SDK version strategy #1250

Are you sure you want to change the base?

feat: Update Operator SDK version strategy #1250

Conversation

mateusoliveira43 commented Dec 1, 2023

openshift-ci bot commented Dec 1, 2023

openshift-ci bot commented Dec 1, 2023

mateusoliveira43 Dec 1, 2023

Choose a reason for hiding this comment

mateusoliveira43 commented Dec 4, 2023

mateusoliveira43 Dec 4, 2023

Choose a reason for hiding this comment

mateusoliveira43 Dec 4, 2023

Choose a reason for hiding this comment

mateusoliveira43 commented Dec 4, 2023

mateusoliveira43 Dec 5, 2023 • edited

Choose a reason for hiding this comment

mateusoliveira43 commented Dec 5, 2023

kaovilai Dec 6, 2023

Choose a reason for hiding this comment

mateusoliveira43 Dec 6, 2023

Choose a reason for hiding this comment

mpryc commented Dec 6, 2023

mateusoliveira43 commented Dec 6, 2023

openshift-ci bot commented Feb 10, 2024

openshift-merge-robot commented Feb 10, 2024

openshift-bot commented May 10, 2024

mateusoliveira43 commented May 10, 2024

mateusoliveira43 Dec 5, 2023 •

edited