-
Notifications
You must be signed in to change notification settings - Fork 20
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
File based catalog #201
File based catalog #201
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #201 +/- ##
=======================================
Coverage 61.78% 61.78%
=======================================
Files 2 2
Lines 785 785
=======================================
Hits 485 485
Misses 249 249
Partials 51 51
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.
some nits left, but overall LGTM
@@ -208,10 +209,9 @@ jobs: | |||
image: ${{ env.OPERATOR_NAME }}-catalog | |||
tags: ${{ env.IMG_TAGS }} | |||
platforms: linux/amd64,linux/arm64 |
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.
not related to this PR, but I do not think that catalog image needs to be multi-platform. It's just a single yaml file!
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.
Could pulling the image from a different platform fail if only amd64 is available (despite being just a single yaml inside)?
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.
The image is being pulled by OLM. So, not being entirely sure, this needs to be checked first 👍
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.
From OS docs:
Set --filter-by-os to the operating system and architecture to use for the base image, which must match the target OpenShift Container Platform cluster. Valid values are linux/amd64, linux/ppc64le, and linux/s390x.
Which not sure if it could be interpreted to also the actual catalog yaml img that it's delivered
Signed-off-by: dd di cesare <[email protected]>
* Changing OPM tool installation * Adding needed ENV vars Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
58d4c08
to
26630ea
Compare
config/deploy/olm/kustomization.yaml
Outdated
@@ -0,0 +1,8 @@ | |||
# Adds namespace to all resources. | |||
namespace: authorino-system |
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.
For consistency with the other supported installation methods:
namespace: authorino-system | |
namespace: authorino-operator |
config/deploy/olm/namespace.yaml
Outdated
metadata: | ||
labels: | ||
control-plane: controller-manager | ||
name: system |
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.
name: system | |
name: operator |
config/deploy/olm/subscription.yaml
Outdated
name: kuadrant | ||
spec: | ||
source: authorino-operator-catalog | ||
sourceNamespace: authorino-system |
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.
sourceNamespace: authorino-system | |
sourceNamespace: authorino-operator |
config/deploy/olm/subscription.yaml
Outdated
source: authorino-operator-catalog | ||
sourceNamespace: authorino-system | ||
name: authorino-operator | ||
channel: "preview" |
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.
We decided using stable
as the default channel. Even if this ends up overridden by command-line args, I think having preview
could be confusing.
channel: "preview" | |
channel: "stable" |
--- | ||
schema: olm.channel | ||
package: authorino-operator | ||
name: preview |
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.
name: preview | |
name: stable |
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 know it's just a template, but to avoid confusion.
Signed-off-by: dd di cesare <[email protected]>
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.
Thanks, @didierofrivia!
This PR replaces the old catalog generation sql based for the file based. This is due some limitations (semver check that doesn't allow beta tags) and deprecation notice:
time="2024-08-26T13:47:11Z" level=warning msg="\x1b[1;33mDEPRECATION NOTICE:\nSqlite-based catalogs and their related subcommands are deprecated. Support for\nthem will be removed in a future release. Please migrate your catalog workflows\nto the new file-based catalog format.\x1b[0m"
Verification steps
Build local catalog.
Push the image to Quay.io.
Create kind cluster
Deploy OLM
Deploy authorino operator using OLM
Check the authorino operator is up and running (it takes up to few minutes)