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

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

Merged

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Jan 29, 2025

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 owner January 29, 2025 12:44
Copy link

netlify bot commented Jan 29, 2025

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit ed68780
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/679a65d81ce80c00085e9749
😎 Deploy Preview https://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.

@camilamacedo86 camilamacedo86 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 properly Jan 29, 2025
Copy link

codecov bot commented Jan 29, 2025

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           
Flag Coverage Δ
e2e 53.21% <ø> (-0.09%) ⬇️
unit 54.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.

Copy link
Member

@rashmigottipati rashmigottipati 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 Indicates that a PR is ready to be merged. label Jan 29, 2025
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2025
@camilamacedo86 camilamacedo86 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 rc2 Jan 29, 2025
@camilamacedo86 camilamacedo86 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-rc1 Jan 29, 2025
Makefile Outdated
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
Copy link
Contributor

@tmshort tmshort Jan 29, 2025

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
Contributor Author

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
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
Contributor Author

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?

Copy link
Member

@LalatenduMohanty LalatenduMohanty 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 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-ci openshift-ci bot 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
Copy link
Member

@LalatenduMohanty LalatenduMohanty left a comment

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-ci openshift-ci bot 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 7ee4ced Jan 29, 2025
22 checks passed
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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants