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

WINC-751: Upgrade sdk-operator to 1.32.0 #1912

Merged
merged 14 commits into from
Nov 8, 2023

Conversation

alinaryan
Copy link
Contributor

@alinaryan alinaryan commented Oct 19, 2023

Updates the OSDK version to latest v1.32.0.

  • Added updates required for Golang/v3 based operators following https://sdk.operatorframework.io/docs/upgrading-sdk-version/ starting from version 1.16.0 to latest 1.32.0.

  • Updates the OSDK version in our pre-release.sh script and the hacking script.

  • Skipped anything that did not apply to our operator (helm/ansible/non-Go-v3 based operators)

@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 19, 2023

@alinaryan: This pull request references WINC-751 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

WIP PR for sdk upgrade

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-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 19, 2023
@openshift-ci 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 Oct 19, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 19, 2023

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

@alinaryan alinaryan changed the title WIP: WINC-751: Upgrade sdk-operator to v1.23.0 WIP: WINC-751: Upgrade sdk-operator to latest Oct 19, 2023
@alinaryan alinaryan force-pushed the OSDK-UPGRADE branch 9 times, most recently from 576026a to 03db602 Compare November 1, 2023 15:11
@alinaryan
Copy link
Contributor Author

/test build

@alinaryan
Copy link
Contributor Author

/test aws-e2e-operator

@openshift-ci-robot
Copy link

openshift-ci-robot commented Nov 1, 2023

@alinaryan: This pull request references WINC-751 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

Updates the OSDK version from 1.15.0 to the latest 1.32.0.
Followed https://sdk.operatorframework.io/docs/upgrading-sdk-version/ from version 1.16.0 to 1.32.0 and added updates required for Go/v3 based operators.

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.

@alinaryan alinaryan changed the title WIP: WINC-751: Upgrade sdk-operator to latest WINC-751: Upgrade sdk-operator to latest Nov 1, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Nov 1, 2023

@alinaryan: This pull request references WINC-751 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

Updates the OSDK version from 1.15.0 to the latest 1.32.0.
Followed https://sdk.operatorframework.io/docs/upgrading-sdk-version/ from version 1.16.0 to 1.32.0 and added updates required for Go/v3 based operators.

Updates the OSDK version in our pre-release.sh hack script and the README.md

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-ci-robot
Copy link

openshift-ci-robot commented Nov 1, 2023

@alinaryan: This pull request references WINC-751 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

Updates the OSDK version from 1.15.0 to the latest 1.32.0.
Followed https://sdk.operatorframework.io/docs/upgrading-sdk-version/ from version 1.16.0 to 1.32.0 and added updates required for Go/v3 based operators.

Updates the OSDK version in our pre-release.sh script and the hacking script.

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-ci-robot
Copy link

openshift-ci-robot commented Nov 1, 2023

@alinaryan: This pull request references WINC-751 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

Updates the OSDK version from v1.15.0 to the latest v1.32.0.

Added updates required for Golang/v3 based operators following https://sdk.operatorframework.io/docs/upgrading-sdk-version/ starting from version 1.16.0 to latest 1.32.0.

Updates the OSDK version in our pre-release.sh script and the hacking script.

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-ci-robot
Copy link

openshift-ci-robot commented Nov 1, 2023

@alinaryan: This pull request references WINC-751 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

Updates the OSDK version to latest v1.32.0.

Added updates required for Golang/v3 based operators following https://sdk.operatorframework.io/docs/upgrading-sdk-version/ starting from version 1.16.0 to latest 1.32.0.

Updates the OSDK version in our pre-release.sh script and the hacking script.

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.

@aravindhp
Copy link
Contributor

/test images
/test aws-e2e-operator
/test aws-e2e-upgrade

Copy link
Contributor

@aravindhp aravindhp left a comment

Choose a reason for hiding this comment

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

I did not do a full review but noticed that you missed updating get_operator_sdk(). Please fix and kick off an e2e that exercises that function.

@@ -227,7 +227,7 @@ github_update() {

# remove make bundle artifacts
sed -i 's/REPLACE_IMAGE:latest/REPLACE_IMAGE/' bundle/manifests/windows-machine-config-operator.clusterserviceversion.yaml
sed -i 's/operator-sdk-v1.14.0+git/operator-sdk-v1.15.0+git/' bundle/manifests/windows-machine-config-operator.clusterserviceversion.yaml
sed -i 's/operator-sdk-v1.14.0+git/operator-sdk-v1.32.0+git/' bundle/manifests/windows-machine-config-operator.clusterserviceversion.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the OPERATOR_SDK_VERSION variable here so that we don't need to update this line for future SKD version changes

@jrvaldes
Copy link
Contributor

jrvaldes commented Nov 2, 2023

@alinaryan consider mentioning explicitly the version you are updating to i.e. 1.32 in the PR title instead of "latest"

@@ -47,6 +47,7 @@ else
GOBIN=$(shell go env GOBIN)
endif

.PHONY: all
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these changes required for the sdk-operator upgrade? If so, please mention why in the commit message; otherwise, I recommend moving them to a separate PR.

Copy link
Contributor Author

@alinaryan alinaryan Nov 2, 2023

Choose a reason for hiding this comment

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

Yes, they are required as part of the OSDK upgrade. See https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.16.0/#add-phony-targets-to-makefile

It does not specify why the change is needed so I'm not sure what I'd put in the commit message other than "required by OSDK upgrade" which feels redundant since that's the purpose of almost all the commits in the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add such links to your commit message. This will explain WHY you are making the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add links to this and the rest of the commits

Copy link
Contributor

@saifshaikh48 saifshaikh48 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, just a few minor comments

Makefile Outdated
@@ -108,6 +108,9 @@ build-daemon:
run: manifests generate fmt vet ## Run a controller from your host.
go run cmd/operator/main.go

ifndef ignore-not-found
ignore-not-found = false
Copy link
Contributor

Choose a reason for hiding this comment

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

consider making this all caps like out other make constants

@@ -15,7 +15,7 @@ spec:
- "--secure-listen-address=0.0.0.0:8443"
- "--upstream=http://127.0.0.1:8080/"
- "--logtostderr=true"
- "--v=10"
- "--v=0"
Copy link
Contributor

Choose a reason for hiding this comment

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

did they give any reason why this change is recommended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but I'll add the link to the doc in commit message

Comment on lines +180 to +193
# You can enable this value if you would like to use SHA Based Digests
# To enable set flag to true
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this should go in the commit message to explain why this change is made

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

Comment on lines +150 to +151
KUSTOMIZE_VERSION ?= v4.5.5
CONTROLLER_TOOLS_VERSION ?= v0.10.0
Copy link
Contributor

Choose a reason for hiding this comment

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

since these were introduced in the last commit, I recommend squashing this commit

@@ -154,17 +154,17 @@ KUSTOMIZE_INSTALL_SCRIPT ?= "https://raw.githubusercontent.com/kubernetes-sigs/k
.PHONY: kustomize
kustomize: $(KUSTOMIZE) ## Download kustomize locally if necessary.
$(KUSTOMIZE): $(LOCALBIN)
curl -s $(KUSTOMIZE_INSTALL_SCRIPT) | bash -s -- $(subst v,,$(KUSTOMIZE_VERSION)) $(LOCALBIN)
test -s $(LOCALBIN)/kustomize || { curl -s $(KUSTOMIZE_INSTALL_SCRIPT) | bash -s -- $(subst v,,$(KUSTOMIZE_VERSION)) $(LOCALBIN); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as previous comment, I recommend squashing this commit into [build] Replace go-get with go-install

@alinaryan alinaryan changed the title WINC-751: Upgrade sdk-operator to latest WINC-751: Upgrade sdk-operator to 1.32.0 Nov 2, 2023
Reduces the debug log level for the sidecar container kube-rbac-proxy from
10 to 0.
https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.17.0/#reduce-debug-log-level-for-the-sidecar-container-kube-rbac-proxy-from-10-to-0

Signed-off-by: Alina Ryan <aliryan@redhat.com>
When using a custom ServiceAccount for deployment, the watch role is
now required.

https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.31.0/#require-watch-on-secrets

Manually added ServiceAccount permissions to cmd/operator/main.go

Ran:
'make bundle'

Removed:
'latest:' tag from REPLACE_IMAGE

Added back required LABELS in the bundle.Dockerfile that were removed
when 'make bundle' was run.

Added back 'com.redhat.openshift.versions:' line to
bundle/metadata/annotations.yaml

Signed-off-by: Alina Ryan <aliryan@redhat.com>
Updates the OSDK version from 1.15.0 to 1.32.0
in common.sh and pre-release.sh

Signed-off-by: Alina Ryan <aliryan@redhat.com>
Adjusts the recommended OSDK install version from 1.15.0
to 1.32.0 in the prerequisites.

Signed-off-by: Alina Ryan <aliryan@redhat.com>
The controller gen tool considers every file in the directory tree by default
when generating CRDs and RBAC. This caused an issue when running
'make bundle' with the new OSDK version as the CONTROLLER_GEN
tool was also generating files for our submodules. This commit fixes the
problem by only specifying the cmd, controllers, and pkg paths in the
'make manifests' and 'make generate' commands.

Signed-off-by: Alina Ryan <aliryan@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2023
@jrvaldes
Copy link
Contributor

jrvaldes commented Nov 8, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2023
@sebsoto
Copy link
Contributor

sebsoto commented Nov 8, 2023

/approve

Copy link
Contributor

openshift-ci bot commented Nov 8, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alinaryan, sebsoto

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2023
@alinaryan alinaryan marked this pull request as ready for review November 8, 2023 14:54
@openshift-ci 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 Nov 8, 2023
@openshift-ci-robot
Copy link

/test remaining-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 4e31915 and 2 for PR HEAD 82c730e in total

@alinaryan
Copy link
Contributor Author

/test aws-e2e-upgrade

@sebsoto
Copy link
Contributor

sebsoto commented Nov 8, 2023

/override ci/prow/nutanix-e2e-operator

Copy link
Contributor

openshift-ci bot commented Nov 8, 2023

@sebsoto: Overrode contexts on behalf of sebsoto: ci/prow/nutanix-e2e-operator

In response to this:

/override ci/prow/nutanix-e2e-operator

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.

Copy link
Contributor

openshift-ci bot commented Nov 8, 2023

@alinaryan: 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-bot openshift-merge-bot bot merged commit c8a5199 into openshift:master Nov 8, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants