Skip to content

✨ OPRUN-3731: Move catalogd entrypoint #1802

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
Merged
merged 1 commit into from
Feb 25, 2025
Merged

✨ OPRUN-3731: Move catalogd entrypoint #1802

merged 1 commit into from
Feb 25, 2025

Conversation

@OchiengEd
Copy link
Contributor

Description

This pull request aims to move the catalogd entrypoint from operator-controller/catalogd/cmd/catalogd to operator-controller/cmd/catalogd.

The above action is a continuation of the monorepo work detailed in issue #1340

While this PR achieves the high level task, more work is ongoing to optimize the makefile to re-use for instance the build command.

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

Related to #1340

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 24, 2025
@netlify
Copy link

netlify bot commented Feb 24, 2025 •
edited
Loading

✅ Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 93d62dc
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/67be15a9c1fb7d0008ab942f
😎 Deploy Preview https://deploy-preview-1802--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 reviewed Feb 24, 2025
View reviewed changes
camilamacedo86
camilamacedo86 reviewed Feb 24, 2025
View reviewed changes
camilamacedo86
camilamacedo86 reviewed Feb 24, 2025
View reviewed changes
camilamacedo86
camilamacedo86 reviewed Feb 24, 2025
View reviewed changes
camilamacedo86
camilamacedo86 reviewed Feb 24, 2025
View reviewed changes
@OchiengEd OchiengEd force-pushed the move_catalogd_entrypoint branch from dd73555 to 0ed91d1 Compare February 24, 2025 19:24
@codecov
Copy link

codecov bot commented Feb 24, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.34%. Comparing base (5107960) to head (93d62dc).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1802   +/-   ##
=======================================
  Coverage   68.34%   68.34%           
=======================================
  Files          62       62           
  Lines        5117     5117           
=======================================
  Hits         3497     3497           
  Misses       1390     1390           
  Partials      230      230
Flag Coverage Δ
e2e 51.62% <ø> (ø)
unit 55.91% <ø> (ø)

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.

tmshort
tmshort reviewed Feb 25, 2025
View reviewed changes
Makefile Outdated
Comment on lines 27 to 31
CATALOGD_BINARIES=catalogd
LINUX_BINARIES=$(join $(addprefix linux/,$(CATALOGD_BINARIES)), )

BUILDCMD = go build -tags '$(GO_BUILD_TAGS)' -ldflags '$(GO_BUILD_LDFLAGS)' -gcflags '$(GO_BUILD_GCFLAGS)' -asmflags '$(GO_BUILD_ASMFLAGS)' -o $(BUILDBIN)/$(notdir $@) ./cmd/$(notdir $@)

Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be necessary, as catalogd and operator-controller binaries should be able to use the same set of build rules/recipes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be resolved.

Copy link
Member

Choose a reason for hiding this comment

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

Still need to remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

those have been cleaned up.

tmshort
tmshort reviewed Feb 25, 2025
View reviewed changes
@OchiengEd OchiengEd force-pushed the move_catalogd_entrypoint branch 2 times, most recently from fa2de51 to f304e62 Compare February 25, 2025 16:58
camilamacedo86
camilamacedo86 reviewed Feb 25, 2025
View reviewed changes
@OchiengEd OchiengEd marked this pull request as ready for review February 25, 2025 17:17
@OchiengEd OchiengEd requested a review from a team as a code owner February 25, 2025 17:17
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2025
joelanford
joelanford reviewed Feb 25, 2025
View reviewed changes
joelanford
joelanford reviewed Feb 25, 2025
View reviewed changes
Makefile Outdated

.PHONY: kind-load
kind-load: $(KIND) #EXHELP Loads the currently constructed images into the KIND cluster.
$(CONTAINER_RUNTIME) save $(IMG) | $(KIND) load image-archive /dev/stdin --name $(KIND_CLUSTER_NAME)
Copy link
Member

@joelanford joelanford Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

IMG should be renamed to OPCON_IMG or OPERATOR_CONTROLLER_IMG for symmetry.

It probably also makes sense to change IMAGE_REPO into IMG_NAMESPACE (or similar), and then have:

ifeq ($(origin IMG_NAMESPACE), undefined)
IMG_NAMESPACE := quay.io/operator-framework
endif
export IMG_NAMESPACE

ifeq ($(origin IMAGE_TAG), undefined)
IMAGE_TAG := devel
endif
export IMAGE_TAG

OPCON_IMG :=$(IMG_NAMESPACE)/operator-controller:$(IMAGE_TAG)
CATALOGD_IMG := $(IMG_NAMESPACE)/catalogd:$(IMAGE_TAG)

Copy link
Contributor Author

@OchiengEd OchiengEd Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It is possible to introduce the IMG_NAMESPACE variable however, it seems that the OPERATOR_CONTROLLER_IMAGE_REPO and CATALOGD_IMAGE_REPO variables may need to stay around as they are passed to the release target in the make file. See

operator-controller/Makefile

Line 353 in 5107960

OPERATOR_CONTROLLER_IMAGE_REPO=$(IMAGE_REPO) CATALOGD_IMAGE_REPO=$(CATALOG_IMAGE_REPO) $(GORELEASER) $(GORELEASER_ARGS)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Let me know if the current solution works

@OchiengEd OchiengEd force-pushed the move_catalogd_entrypoint branch 2 times, most recently from 8d867d8 to 0d8387e Compare February 25, 2025 19:06
@OchiengEd
Move catalogd entrypoint to cmd/catalogd
93d62dc 
Move catalogd entrypoint and merge catalogd and operator-controller
make files

Signed-off-by: Edmund Ochieng <[email protected]>
@OchiengEd OchiengEd force-pushed the move_catalogd_entrypoint branch from 0d8387e to 93d62dc Compare February 25, 2025 19:10
camilamacedo86
camilamacedo86 reviewed Feb 25, 2025
View reviewed changes
Makefile
ifeq ($(origin IMAGE_REPO), undefined)
IMAGE_REPO := quay.io/operator-framework/operator-controller
ifeq ($(origin IMG_NAMESPACE), undefined)
IMG_NAMESPACE := quay.io/operator-framework
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer REGISTRY
instead since we publish the image into the registry
but I do not think that it is a blocker for this one.

Copy link
Contributor

@tmshort tmshort Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I like REGISTRY better, too, but we can update later

Copy link
Member

Choose a reason for hiding this comment

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

+1 agree with @camilamacedo86

camilamacedo86
camilamacedo86 approved these changes Feb 25, 2025
View reviewed changes
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm

@LalatenduMohanty @tmshort ?

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2025
@tmshort
Copy link
Contributor

tmshort commented Feb 25, 2025

/hold
A quick hold while I review

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 25, 2025
tmshort
tmshort approved these changes Feb 25, 2025
View reviewed changes
Copy link
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

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

/unhold
/lgtm

I personally prefer OPCON and CATD for some of these variable names, to make them shorter and closer in length to each other, but it's not a blocker.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 25, 2025
@tmshort tmshort added this pull request to the merge queue Feb 25, 2025
Merged via the queue into operator-framework:main with commit 3921e24 Feb 25, 2025
20 checks passed
@LalatenduMohanty
Copy link
Member

Looks like we left the dockerignore file in catalogd/ https://github.com/operator-framework/operator-controller/blob/main/catalogd/.dockerignore , sending a PR for this.

@LalatenduMohanty
Copy link
Member

PR to move the dockerignore fine #1813

@OchiengEd OchiengEd deleted the move_catalogd_entrypoint branch February 25, 2025 22:06
@tmshort tmshort changed the title ✨ Move catalogd entrypoint ✨ OPRUN-3731: Move catalogd entrypoint Feb 26, 2025
@tmshort tmshort mentioned this pull request Mar 4, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joelanford joelanford joelanford left review comments

@LalatenduMohanty LalatenduMohanty LalatenduMohanty left review comments

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@tmshort tmshort tmshort approved these changes

Assignees

@camilamacedo86 camilamacedo86

@tmshort tmshort

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@OchiengEd @tmshort @LalatenduMohanty @joelanford @camilamacedo86