Skip to content

🌱 Move Registry YAML and remove use of namespace in kustomizations #2088

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

Closed
wants to merge 2 commits into from

Conversation

tmshort
Copy link
Contributor

@tmshort tmshort commented Jul 10, 2025

Move e2e registry resources into config

Currently, the e2e registry resources are located in a script testdata/build-test-registry.sh

The resources in this script can be moved the the e2e config and manifests that now exist.

The only thing that remains is the job, that kinda-sorta needs to wait until the registry is up and working.

This requires removing the use of namespace in kustomize, because it behaves oddly when components and overlays use it.

Description

Reviewer Checklist

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

@tmshort tmshort requested a review from a team as a code owner July 10, 2025 21:28
Copy link

netlify bot commented Jul 10, 2025

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 427a442
🔍 Latest deploy log https://app.netlify.com/projects/olmv1/deploys/68716b0427bc8f0008005a01
😎 Deploy Preview https://deploy-preview-2088--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 project configuration.

Copy link

openshift-ci bot commented Jul 10, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kevinrizza for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

Copy link

codecov bot commented Jul 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.52%. Comparing base (134f887) to head (427a442).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2088      +/-   ##
==========================================
- Coverage   73.52%   73.52%   -0.01%     
==========================================
  Files          78       78              
  Lines        7241     7240       -1     
==========================================
- Hits         5324     5323       -1     
  Misses       1566     1566              
  Partials      351      351              
Flag Coverage Δ
e2e 43.89% <ø> (-0.01%) ⬇️
experimental-e2e 50.23% <ø> (-0.01%) ⬇️
unit 58.74% <ø> (-0.01%) ⬇️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tmshort added 2 commits July 11, 2025 15:50
Currently, the e2e registry resources are located in a script
`testdata/build-test-registry.sh`

The resources in this script can be moved the the e2e config and
manifests that now exist.

The only thing that remains is the job, that kinda-sorta needs
to wait until the registry is up and working.

Signed-off-by: Todd Short <[email protected]>
@tmshort tmshort force-pushed the move-registry-yaml branch from 5c0b959 to 427a442 Compare July 11, 2025 19:50
@tmshort tmshort mentioned this pull request Jul 11, 2025
4 tasks
@@ -1,7 +1,6 @@
# Does not include the CRD, which must be added separately (it's non-namespaced)
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
namespace: olmv1-system
Copy link
Contributor

Choose a reason for hiding this comment

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

So, if we need to change the name, do we have to update all the files after this change?
Also, I haven't checked, but in the downstream setup, are we using a shell script to make the change, or are we properly setting the Kustomize value and then building?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we not do the other changes and keep the namespace definition on the kustomize?

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 is making the kustomization more robust.

Downstream, the kustomization is done with a completely different overlay, and thus, sets the namespace appropriately according to openshift.

It's non-trivial to change the namespace via kustomize; there are also odd interdependencies based on ordering of components that may cause the namespaces of components outside of olmv1-system to be overwritten. For example, the CA components need(ed/s) to be last, or their cert-manager namespace will be overwritten with olmv1-system. Downstream doesn't have to deal with cert-manager, so there's no issues with namespace overwriting.

This change is something @joelanford had been looking at, in his reorganization of kustomize (tagging him as he has some interest here).

We never really gave people an easy way to change the namespace anyways, as the installation was done via the quickstart-generated files, and those don't have options to change the namespace.

If we want people to change the namespace, perhaps we should add functionality to the resulting install.sh script?

Copy link
Contributor

@camilamacedo86 camilamacedo86 Jul 14, 2025

Choose a reason for hiding this comment

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

This is my only concern in the PR. That said, if everyone is aligned with the change, I’m okay moving forward with it.

If we want to provide this flexibility, we’ll need to replace the namespace (ns) in all places eventually—regardless of whether we use Kustomize to build it or not.

The only downside I see is that this information isn't currently centralised, so we’ll need to remember to update every reference manually. That said, a simple find-and-replace should be enough to handle it if changes are needed later. We can either add validation to ensure/ci if we wish to do so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There has been some discussion on moving the configuration from kustomize to helm... but that's a project in-and-of itself...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm having problems with the upgrade-e2e, at this point, I want to split this into two PRs, the namespace change and the registry change.

@tmshort
Copy link
Contributor Author

tmshort commented Jul 14, 2025

Closing to separate the PR into two parts.

@tmshort tmshort closed this Jul 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants