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 operator-sdk to local bin #308

Merged

Conversation

yazug
Copy link
Contributor

@yazug yazug commented Dec 13, 2023

specify version for operator-sdk for make bundle
download operator-sdk pinned version when generating bundle update make bundle to use local operator-sdk
update to 1.31 version

@openshift-ci openshift-ci bot requested review from dprince and eharney December 13, 2023 20:14
Makefile Outdated Show resolved Hide resolved
@yazug yazug force-pushed the standardize_makefile branch from 886ef2d to 5bba07a Compare December 18, 2023 20:01
@yazug
Copy link
Contributor Author

yazug commented Dec 19, 2023

/test cinder-operator-build-deploy-tempest

2 similar comments
@yazug
Copy link
Contributor Author

yazug commented Dec 20, 2023

/test cinder-operator-build-deploy-tempest

@yazug
Copy link
Contributor Author

yazug commented Dec 21, 2023

/test cinder-operator-build-deploy-tempest

@@ -245,27 +249,48 @@ CONTROLLER_TOOLS_VERSION ?= v0.11.1

KUSTOMIZE_INSTALL_SCRIPT ?= "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh"
.PHONY: kustomize
kustomize: $(KUSTOMIZE) ## Download kustomize locally if necessary.
kustomize: $(KUSTOMIZE) ## Download kustomize locally if necessary. If wrong version is installed, it will be removed before downloading.
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this locally, and the version check only executes if the $(KUSTOMIZE) target is phony. You can add this as a separate line, or append "$(KUSTOMIZE)" to L251.

Copy link
Contributor

Choose a reason for hiding this comment

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

Argh, I hate github's PR interface.

But you get the idea: you need to make the KUSTOMIZE target phony.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, noted but this is pulled directly from the skeleton Makefile generated by operator-sdk and I would prefer not to include other improvements in these standardize_makefile pull requests unless it is an issue with how it is currently?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is an issue, because the version logic doesn't get triggered unless KUSTOMIZE is phony:

# Start clean
[cinder-operator]$ rm bin/kustomize

# Install an old version (v3.8.6)
[cinder-operator]$ KUSTOMIZE_VERSION=v3.8.6 make kustomize
test -s /home/abishop/cinder-operator/bin/kustomize || { curl -Ss "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" | bash -s -- 3.8.6 /home/abishop/cinder-operator/bin; }
{Version:kustomize/v3.8.6 GitCommit:c1747439cd8bc956028ad483cdb30d9273c18b24 BuildDate:2020-10-29T23:07:50Z GoOs:linux GoArch:amd64}
kustomize installed to /home/abishop/cinder-operator/bin/kustomize

[cinder-operator]$ bin/kustomize version
{Version:kustomize/v3.8.6 GitCommit:c1747439cd8bc956028ad483cdb30d9273c18b24 BuildDate:2020-10-29T23:07:50Z GoOs:linux GoArch:amd64}

# This should trigger the upgrade to KUSTOMIZE_VERSION (v3.8.7)
[cinder-operator]$ make kustomize
make: Nothing to be done for 'kustomize'.

# Repeat with phony KUSTOMIZE target. Now the upgrade works as intended.
[cinder-operator]$ git diff
diff --git a/Makefile b/Makefile
index 0e50536..02f39d8 100644
--- a/Makefile
+++ b/Makefile
@@ -248,7 +248,7 @@ KUSTOMIZE_VERSION ?= v3.8.7
 CONTROLLER_TOOLS_VERSION ?= v0.11.1
 
 KUSTOMIZE_INSTALL_SCRIPT ?= "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh"
-.PHONY: kustomize
+.PHONY: kustomize $(KUSTOMIZE)
 kustomize: $(KUSTOMIZE) ## Download kustomize locally if necessary. If wrong version is installed, it will be removed before downloading.
 $(KUSTOMIZE): $(LOCALBIN)
        @if test -x $(LOCALBIN)/kustomize && ! $(LOCALBIN)/kustomize version | grep -q $(KUSTOMIZE_VERSION); then \

[cinder-operator]$ make kustomize
/home/abishop/cinder-operator/bin/kustomize version is not expected v3.8.7. Removing it before installing.
test -s /home/abishop/cinder-operator/bin/kustomize || { curl -Ss "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" | bash -s -- 3.8.7 /home/abishop/cinder-operator/bin; }
{Version:kustomize/v3.8.7 GitCommit:ad092cc7a91c07fdf63a2e4b7f13fa588a39af4f BuildDate:2020-11-11T23:14:14Z GoOs:linux GoArch:amd64}
kustomize installed to /home/abishop/cinder-operator/bin/kustomize

[cinder-operator]$ bin/kustomize version
{Version:kustomize/v3.8.7 GitCommit:ad092cc7a91c07fdf63a2e4b7f13fa588a39af4f BuildDate:2020-11-11T23:14:14Z GoOs:linux GoArch:amd64}

yazug added 2 commits January 15, 2024 13:07
specify version for operator-sdk for make bundle
download operator-sdk pinned version when generating bundle
update make bundle to use local operator-sdk
update to 1.31 version
sync from skeleton from operator-sdk init

Signed-off-by: Jon Schlueter <[email protected]>
@yazug yazug force-pushed the standardize_makefile branch from 5bba07a to 27d03ae Compare January 15, 2024 20:36
Copy link
Contributor

@ASBishop ASBishop left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 25, 2024
Copy link
Contributor

openshift-ci bot commented Jan 25, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ASBishop, yazug

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-merge-bot openshift-merge-bot bot merged commit 022d50b into openstack-k8s-operators:main Jan 25, 2024
7 checks passed
ASBishop pushed a commit to ASBishop/cinder-operator that referenced this pull request Mar 11, 2024
…faulting

Move ironic template defaulting to the webhook
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants