-
Notifications
You must be signed in to change notification settings - Fork 58
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
🐛 fix release artefacts after monorepo changes and release candidate 1.2.0-rc1 #1661
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
00522ff
to
aec8ce2
Compare
Makefile
Outdated
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
aec8ce2
to
ed68780
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
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:
Following are the details...
TL'DR:
After the release candidate v1.2.0-rc1, we could check that the installer is falling with
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:
Note that the images were generated:
However, we need to replace the tag devel with the version.