Skip to content

🐛 fix release artefacts after monorepo changes and release candidate 1.2.0-rc1 - #1661

Merged
camilamacedo86 merged 2 commits into
operator-framework:mainfrom
camilamacedo86:fix-installer-released
Jan 29, 2025
Merged

🐛 fix release artefacts after monorepo changes and release candidate 1.2.0-rc1#1661
camilamacedo86 merged 2 commits into
operator-framework:mainfrom
camilamacedo86:fix-installer-released

Conversation

@camilamacedo86

@camilamacedo86camilamacedo86 commented Jan 29, 2025

Copy link
Copy Markdown
Contributor

Description

We have been working on an initiative to integrate the catalogd project into this repository.
As part of this effort, we released a candidate version to validate the changes made so far. During the v1.2.0-rc1 release, we identified the following issues:

  • The installer script was not downloading the operator-controller.yaml file.
  • The images defined for both projects in operator-controller.yaml were using the devel tag instead of the correct released version tag.

Following are the details...

TL'DR:

After the release candidate v1.2.0-rc1, we could check that the installer is falling with

$ curl -L -s https://github.com/operator-framework/operator-controller/releases/latest/download/install.sh | bash -s
namespace/cert-manager unchanged
customresourcedefinition.apiextensions.k8s.io/certificaterequests.cert-manager.io unchanged
customresourcedefinition.apiextensions.k8s.io/certificates.cert-manager.io unchanged
customresourcedefinition.apiextensions.k8s.io/challenges.acme.cert-manager.io unchanged
customresourcedefinition.apiextensions.k8s.io/clusterissuers.cert-manager.io unchanged
customresourcedefinition.apiextensions.k8s.io/issuers.cert-manager.io unchanged
customresourcedefinition.apiextensions.k8s.io/orders.acme.cert-manager.io unchanged
serviceaccount/cert-manager-cainjector unchanged
serviceaccount/cert-manager unchanged
serviceaccount/cert-manager-webhook unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-cainjector unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-controller-issuers unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-controller-clusterissuers unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-controller-certificates unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-controller-orders unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-controller-challenges unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-controller-ingress-shim unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-cluster-view unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-view unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-edit unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-controller-approve:cert-manager-io unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-controller-certificatesigningrequests unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-webhook:subjectaccessreviews unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-cainjector unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-controller-issuers unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-controller-clusterissuers unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-controller-certificates unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-controller-orders unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-controller-challenges unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-controller-ingress-shim unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-controller-approve:cert-manager-io unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-controller-certificatesigningrequests unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-webhook:subjectaccessreviews configured
role.rbac.authorization.k8s.io/cert-manager-cainjector:leaderelection unchanged
role.rbac.authorization.k8s.io/cert-manager:leaderelection unchanged
role.rbac.authorization.k8s.io/cert-manager-webhook:dynamic-serving unchanged
rolebinding.rbac.authorization.k8s.io/cert-manager-cainjector:leaderelection unchanged
rolebinding.rbac.authorization.k8s.io/cert-manager:leaderelection configured
rolebinding.rbac.authorization.k8s.io/cert-manager-webhook:dynamic-serving configured
service/cert-manager unchanged
service/cert-manager-webhook unchanged
deployment.apps/cert-manager-cainjector unchanged
deployment.apps/cert-manager unchanged
deployment.apps/cert-manager-webhook unchanged
mutatingwebhookconfiguration.admissionregistration.k8s.io/cert-manager-webhook configured
validatingwebhookconfiguration.admissionregistration.k8s.io/cert-manager-webhook configured
deployment.apps/cert-manager-webhook condition met
deployment.apps/cert-manager-cainjector condition met
deployment.apps/cert-manager condition met
mutatingwebhookconfigurations/cert-manager-webhook has {.webhooks[0].clientConfig.caBundle}.
validatingwebhookconfigurations/cert-manager-webhook has {.webhooks[0].clientConfig.caBundle}.
error: the path "./operator-controller.yaml" does not exist

Note that we are reverting the change in the quick-start target to be as it was before the minor changes in this line, which should sort out the above scenario. See: https://github.com/operator-framework/operator-controller/blob/v1.1.0/Makefile#L314-L319

Then, if we manually download the operator-controller.ymal we will check that it will fail because did not find the images with the devel tag. The operator-controller.yaml was built with the tag image devel for the images instead tag. See:

$ cat operator-controller.yaml | grep 'quay.io/operator-framework/'
image: quay.io/operator-framework/operator-controller:devel
image: quay.io/operator-framework/catalogd:devel

Note that the images were generated:

However, we need to replace the tag devel with the version.

@camilamacedo86
camilamacedo86 requested a review from a team as a code ownerJanuary 29, 2025 12:44
@netlify

netlifyBot commented Jan 29, 2025

Copy link
Copy Markdown

Deploy Preview for olmv1 ready!

NameLink
🔨 Latest commited68780
🔍 Latest deploy loghttps://app.netlify.com/sites/olmv1/deploys/679a65d81ce80c00085e9749
😎 Deploy Previewhttps://deploy-preview-1661--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@camilamacedo86camilamacedo86 changed the title 🐛 fix: the reference in the quick-star to get the latest release version properly🐛 fix: the reference in the quick-start to get the latest release version properlyJan 29, 2025
@codecov

codecovBot commented Jan 29, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.42%. Comparing base (158d974) to head (ed68780).
Report is 2 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #1661 +/- ##
=======================================
Coverage 67.42% 67.42% =======================================
Files 55 55 Lines 4632 4632 =======================================
Hits 3123 3123 Misses 1284 1284 Partials 225 225 
FlagCoverage Δ
e2e53.21% <ø> (-0.09%)⬇️
unit54.44% <ø> (ø)

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.

rashmigottipati
rashmigottipati previously approved these changes Jan 29, 2025

@rashmigottipatirashmigottipati left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2025
@openshift-ciopenshift-ciBot removed the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2025
@camilamacedo86camilamacedo86 changed the title 🐛 fix: the reference in the quick-start to get the latest release version properly🐛 fix release artefacts after monorepo changes and release candidate rc2Jan 29, 2025
@camilamacedo86camilamacedo86 changed the title 🐛 fix release artefacts after monorepo changes and release candidate rc2🐛 fix release artefacts after monorepo changes and release candidate 1.2.0-rc1Jan 29, 2025
Comment threadMakefile
Comment on lines +327 to 330
quickstart: export MANIFEST := https://github.com/operator-framework/operator-controller/releases/download/$(VERSION)/operator-controller.yaml
quickstart: $(KUSTOMIZE) manifests #EXHELP Generate the unified installation release manifests and scripts.
($(KUSTOMIZE) build $(KUSTOMIZE_BUILD_DIR) && echo "---" && $(KUSTOMIZE) build catalogd/config/overlays/cert-manager | sed "s/cert-git-version/cert-$(VERSION)/g") > $(MANIFEST)
($(KUSTOMIZE) build $(KUSTOMIZE_BUILD_DIR) && echo "---" && $(KUSTOMIZE) build catalogd/config/overlays/cert-manager | sed "s/cert-git-version/cert-$(VERSION)/g" | sed "s/:devel/:$(VERSION)/g") > operator-controller.yaml
envsubst '$$CERT_MGR_VERSION,$$INSTALL_DEFAULT_CATALOGS,$$MANIFEST' < scripts/install.tpl.sh > install.sh

@tmshorttmshortJan 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this is the right thing to do. Because it's pulling the last release version of the manifests into the install script, rather than the current version.
Yes, it's still creating manifests (operator-controller.yaml) from the current (possibly modified code), but the install script will be using the manifests from the specified version. Meaning that the user can't use the quickstart target to install the code they have on disk.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, it's still creating manifests (operator-controller.yaml) from the current (possibly modified code), but the install script will be using the manifests from the specified version.

Note that we are reverting the change in the quick-start target to be as it was before the minor changes in this line, which should sort out the above scenario. See: https://github.com/operator-framework/operator-controller/blob/v1.1.0/Makefile#L314-L319

You can check in the description that causes an error for the install.sh script.
So, it is as it was before. Same behaviour.

meaning that the user can't use the quickstart target to install the code they have on disk.

It was not possible before so we do not need ensure that is possible now.
The user should use make run or do as follow to run the code from the local env.

kind create cluster --name operator-controller
make docker-build kind-load kind-deploy
kind delete cluster --name operator-controller

c/c @joelanford

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, I see it's a revert from #1542 now.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@joelanford@tmshort could we win the LGTM here so we can merge this and do a second RC within to see if that is fixed?

LalatenduMohanty
LalatenduMohanty previously approved these changes Jan 29, 2025

@LalatenduMohantyLalatenduMohanty left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2025
@joelanford
joelanford added this pull request to the merge queue Jan 29, 2025
@openshift-ciopenshift-ciBot removed the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2025
@camilamacedo86
camilamacedo86 removed this pull request from the merge queue due to a manual request Jan 29, 2025

@LalatenduMohantyLalatenduMohanty left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@camilamacedo86
camilamacedo86 added this pull request to the merge queue Jan 29, 2025
@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2025
Merged via the queue into operator-framework:main with commit 7ee4cedJan 29, 2025
camilamacedo86 added a commit that referenced this pull request Jan 29, 2025
…1.2.0-rc1 (#1661) (#1664)
- fix: the reference in the make quickstart to get the latest release version properly
- fix: replace the devel tag image with the version from the release
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtmIndicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@camilamacedo86@LalatenduMohanty@rashmigottipati@tmshort