Skip to content
This repository was archived by the owner on Mar 3, 2025. It is now read-only.

🌱 Combine the catalogd services - #402

Merged
tmshort merged 1 commit into
operator-framework:mainfrom
tmshort:combine-svc
Sep 17, 2024
Merged

🌱 Combine the catalogd services#402
tmshort merged 1 commit into
operator-framework:mainfrom
tmshort:combine-svc

Conversation

@tmshort

@tmshorttmshort commented Sep 16, 2024

Copy link
Copy Markdown
Contributor

Only need to define one service for:

  • metrics
  • webhook
  • catalogd itself

Clean up the name of the service (now catalogd-service) and any corresponding resources

Renumber the ports to 7443/8443/9443. The external port for the catalog server is either 80 or 443.

@tmshort
tmshort requested a review from a team as a code ownerSeptember 16, 2024 21:19
@bentito

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2024
- op: replace
path: /webhooks/0/clientConfig/service/name
value: catalogd-webhook-service
value: catalogd-catalogserver

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it possible to get the name of the service to end up being simply catalogd?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No, because catalogd- is a prefix that is added via kustomize.
The name is defined here: https://github.com/operator-framework/catalogd/blob/main/config/base/manager/catalogserver_service.yaml without the catalogd-. I suppose I could just name it service, that way, it ends up as catalogd-service?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I was able to reduce it to catalogd-service (once the prefix is added).

Comment on lines +17 to +20
- name: webhook
protocol: TCP
port: 444
targetPort: 9444

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

WDYT about this mapping?

  • catalogd api server 8443:443
  • webhook server 9443:9443
  • metrics - 10443:10443

Everything gets a 443 suffix, and the main catalogd api server ends up getting the "nice" https url with no explicit port necessary.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I can play around with it. The metrics port would require some additional golang programming, I'm guessing.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The metrics might be stuck at https due to the proxy.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I don't like 10443, so I used 7443 instead.
The 443 port is patched in with the cert-manager overlay.

@tmshort

Copy link
Copy Markdown
ContributorAuthor

How did this fail EVERYTHING?

Only need to define one service for:
* metrics
* webhook
* catalogd itself
Clean up the name of the service (now `catalogd-service`) and any
corresponding resources
Renumber the ports to 7443/8443/9443. The external port for the catalog
server is either 80 or 443.
Signed-off-by: Todd Short <todd.short@me.com>
@openshift-ciopenshift-ciBot removed the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2024
@codecov

codecovBot commented Sep 17, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 38.04%. Comparing base (2b49a6c) to head (1f73bf8).
Report is 1 commits behind head on main.

Files with missing linesPatch %Lines
cmd/manager/main.go0.00%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #402 +/- ##
=======================================
Coverage 38.04% 38.04% =======================================
Files 15 15 Lines 786 786 =======================================
Hits 299 299 Misses 443 443 Partials 44 44 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LalatenduMohantyLalatenduMohanty left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2024
@tmshort
tmshort added this pull request to the merge queueSep 17, 2024
Merged via the queue into operator-framework:main with commit cdf6c0bSep 17, 2024
@tmshort
tmshort deleted the combine-svc branch September 18, 2024 17:16
@tmshorttmshort mentioned this pull request Sep 18, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtmIndicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@tmshort@bentito@joelanford@LalatenduMohanty