Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to operator-sdk v1.39.1 #115

Open
wants to merge 48 commits into
base: main
Choose a base branch
from
Open

Update to operator-sdk v1.39.1 #115

wants to merge 48 commits into from

Conversation

rajchiluveru
Copy link
Collaborator

No description provided.

@ramperher
Copy link
Collaborator

@rajchiluveru , thanks for starting this!
To address operator-sdk update, typically, we follow these steps, just to make sure that we're using what is expected from the latest release:

  • Create a new operator-sdk project locally, using the latest version
  • Config each operator from scratch. Operator SDK will create a project structure for the given operators
  • Compare the new project created by operator-sdk with what we currently have in the repo
  • Apply the proposed modifications in the new project created

Commands to create the projects with operator-sdk can be found on the README of each operator. Here's the info: https://github.com/openshift-kni/example-cnf?tab=readme-ov-file#how-operators-are-created
And here's the last change that updated operator-sdk: #108. You can see there are plenty of modifications, but all of them comes from the differences between what's generated by operator-sdk and what we have right now.

Copy link
Collaborator

@ramperher ramperher left a comment

Choose a reason for hiding this comment

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

Some changes to be made

LABEL name="NFV Example CNF App MAC Operator" \
maintainer="telcoci" \
vendor="fredco" \
version="v0.2.22" \
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file cannot be modified, you only need to bump the version and release here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still required

@ramperher ramperher dismissed their stale review February 20, 2025 14:09

Off next week, dismissing my review just not to block the merge of this change if working

@ramperher
Copy link
Collaborator

ramperher commented Feb 20, 2025

Just for future reviews, since I'll be off:

  • Take care that the following files from each operator are updated:
    • CHANGELOG.md.
    • Makefile.
    • Dockerfile.
  • Also, make sure that the operator version is within the interval defined in required-annotations.yaml file for olm.skipRange annotation, else update that file to modify the current range.

Here's an example to follow

Also, @rajchiluveru , note the following:

  • Try to keep simple, just apply the main modifications that come from the new operator-sdk version and that are really needed in our operators. Note our Makefiles, Dockerfiles, etc. are more complex than what's generated from operator-sdk, these differences are likely not to be added.
  • Try not to submit too many small commits because each commit triggers the Github action to compile the images. Maybe you're killing a running process if you just commit a minor change immediately. It's better to include all changes in one single commit and then test.
  • Squash your commits once finished.
  • When PR build passes, then test example-cnf with dci-pipeline-check. You need to confirm that the baseline scenario works and that all certification tests have the expected results (verify_tests feature will not fail in that case).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this file deleted?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this file deleted?

LABEL name="NFV Example CNF App MAC Operator" \
maintainer="telcoci" \
vendor="fredco" \
version="v0.2.22" \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still required

# (i.e. docker build --platform linux/arm64 ). However, you must enable docker buildKit for it.
# More info: https://docs.docker.com/develop/develop-images/build_enhancements/
.PHONY: docker-build
docker-build: test ## Build docker image with the manager.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dependency with test task is required, you cannot delete it. That's why the build PR Github action is failing right now.

@@ -220,7 +220,7 @@ deploy: manifests kustomize ## Deploy controller to the K8s cluster specified in
$(KUSTOMIZE) build config/default | $(CLUSTER_CLI) apply -f -

.PHONY: undeploy
undeploy: ## Undeploy controller from the K8s cluster specified in ~/.kube/config. Call with ignore-not-found=true to ignore resource not found errors during deletion.
undeploy: kustomize ## Undeploy controller from the K8s cluster specified in ~/.kube/config. Call with ignore-not-found=true to ignore resource not found errors during deletion.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't delete kustomize dependency as well, this is needed

@@ -34,6 +34,7 @@ import (
"k8s.io/client-go/tools/remotecommand"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/log"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this package really needed? If no function is using it, it's not needed, else a warning will appear when compiling the binary

@@ -1,4 +1,4 @@
FROM quay.io/operator-framework/ansible-operator:v1.36.1
FROM quay.io/operator-framework/ansible-operator:v1.37.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

version/release below need to be bumped too

Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep the original values we have, this doesn't need to be changed.

@@ -7,4 +7,4 @@ metadata:
features.operators.openshift.io/token-auth-aws: "false"
features.operators.openshift.io/token-auth-azure: "false"
features.operators.openshift.io/token-auth-gcp: "false"
olm.skipRange: ">=0.2.22 <=0.2.25"
olm.skipRange: ">=0.2.22 <=0.2.26"
Copy link
Collaborator

Choose a reason for hiding this comment

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

lower range must be updated to 0.2.23 too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants