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.

Sign up for GitHub

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

Jump to bottom

Build file-based catalog #123

Merged
merged 1 commit into from
Mar 5, 2024
Merged

Build file-based catalog #123

merged 1 commit into from
Mar 5, 2024

Conversation

clobrano
Copy link
Contributor

@clobrano clobrano commented Feb 8, 2024

Drop the deprecated db-based catalog in favor of the new file-based catalog.

https://issues.redhat.com/browse/ECOPROJECT-1008

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 8, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clobrano

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-ci openshift-ci bot added the approved label Feb 8, 2024
@clobrano clobrano changed the title Build file-based catalog [WIP] Build file-based catalog Feb 8, 2024
@clobrano
Copy link
Contributor Author

clobrano commented Feb 8, 2024 •
edited
Loading

need to drop few unnecessary commits

It's ok now

@clobrano clobrano changed the title [WIP] Build file-based catalog Build file-based catalog Feb 8, 2024
@clobrano
Copy link
Contributor Author

clobrano commented Feb 8, 2024

/test 4.15-openshift-e2e

1 similar comment
@clobrano
Copy link
Contributor Author

clobrano commented Feb 9, 2024

/test 4.15-openshift-e2e

@clobrano
Copy link
Contributor Author

clobrano commented Feb 13, 2024 •
edited
Loading

Closing this as creating upstream catalog is not necessary

@clobrano clobrano closed this Feb 13, 2024
@clobrano clobrano deleted the build-file-based-catalog-0 branch February 13, 2024 09:01
@clobrano clobrano restored the build-file-based-catalog-0 branch February 22, 2024 09:31
@clobrano clobrano reopened this Feb 22, 2024
@clobrano clobrano requested review from slintes, razo7 and mshitrit and removed request for slintes, razo7 and mshitrit February 22, 2024 09:31
@clobrano clobrano changed the title Build file-based catalog [WIP] Build file-based catalog Feb 22, 2024
@clobrano clobrano changed the title [WIP] Build file-based catalog Build file-based catalog Feb 22, 2024
@clobrano
Copy link
Contributor Author

/test 4.15-openshift-e2e

@clobrano clobrano requested a review from razo7 March 1, 2024 15:54
razo7
razo7 reviewed Mar 4, 2024
View reviewed changes
Copy link
Member

@razo7 razo7 left a comment

Choose a reason for hiding this comment

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

Looks very nice, left some nits

Makefile
.PHONY: catalog-build
catalog-build: opm ## Build a file-based catalog image.
# Remove the catalog directory and Dockerfile if they exist
-rm -r ${CATALOG_DIR} ${CATALOG_DOCKERFILE}
Copy link
Member

Choose a reason for hiding this comment

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

If you first remove the directory then why afterward you would need to delete a file that was in that directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The file isn't inside the directory, it's prefixed with the directory's name
e.g.
catalog_dir and catalog_dir.Dockerfile

Copy link
Member

Choose a reason for hiding this comment

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

Why? Is there a reason for that?
It looks better to have catalog.Dockerfile instead of catalog_dir.Dockerfile similar to the naming format of the bundle Dockerfile that we currently have bundle.Dockerfile
WDYT?

BTW it would be nicer to have this file under the CATALOG_DIR directory if we want to have it as a temporary file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? Is there a reason for that?

opm generate dockerfile works this way [1] 🤷

It looks better to have catalog.Dockerfile

I can change the name of the <catalog_dir> no problem (of course we don't keep this file, but I don't have strong preferences)

[1] https://docs.openshift.com/container-platform/4.14/operators/admin/olm-managing-custom-catalogs.html

Copy link
Member

Choose a reason for hiding this comment

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

CATALOG_DIR := catalog

It is fine, and it would result in catalog.Dockerfile according to steps 1b and 1c in OCP docs.

Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@clobrano
Add Makefile recipe to build a file-based catalog
1ba63fd 
Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
@razo7
Copy link
Member

razo7 commented Mar 5, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 5, 2024
@clobrano
Copy link
Contributor Author

clobrano commented Mar 5, 2024 •
edited
Loading

It doesn't need have CI testing, marking it as ready and then merge right away

@clobrano clobrano marked this pull request as ready for review March 5, 2024 07:50
@clobrano clobrano merged commit 84e63f8 into medik8s:main Mar 5, 2024
2 of 15 checks passed
@openshift-ci openshift-ci bot requested a review from beekhof March 5, 2024 07:51
@clobrano clobrano mentioned this pull request Mar 5, 2024
Closed
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 5, 2024

@clobrano: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/4.15-openshift-e2e 1ba63fd link true /test 4.15-openshift-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@razo7 razo7 razo7 left review comments

@mshitrit mshitrit Awaiting requested review from mshitrit

@slintes slintes Awaiting requested review from slintes

@beekhof beekhof Awaiting requested review from beekhof

Assignees

@razo7 razo7

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@clobrano @razo7 @openshift-merge-robot