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.

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

⚠ ClusterExtension: add installNamespace, remove watchNamespaces #810

Merged
merged 1 commit into from
May 2, 2024

Conversation

joelanford
Copy link
Member

Description

In ClusterExtension:

  • Add spec.installNamespace as a required field. This will be used to configure the context of the client that templates and applies the bundle contents. This will also be necessary for the introduction of a spec.serviceAccount field that operator-controller will use to perform all interactions with the apiserver for that ClusterExtension.
  • Remove spec.watchNamespaces and hardcode all registry+v1 based bundles to watch all namespaces (or fail if they don't support AllNamespaces mode)

Reviewer Checklist

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

@joelanford joelanford requested a review from a team as a code owner May 1, 2024 21:36
Copy link

netlify bot commented May 1, 2024

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 9befb90
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/6633e0c9b105d000087caf4d
😎 Deploy Preview https://deploy-preview-810--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.

@joelanford
Copy link
Member Author

/hold

I want to add a few more tests that are specific to the use of the new InstallNamespace field.

@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 May 1, 2024
Copy link

codecov bot commented May 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.66%. Comparing base (f22ce65) to head (9befb90).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #810      +/-   ##
==========================================
+ Coverage   64.64%   64.66%   +0.01%     
==========================================
  Files          16       16              
  Lines        1321     1316       -5     
==========================================
- Hits          854      851       -3     
+ Misses        404      403       -1     
+ Partials       63       62       -1     
Flag Coverage Δ
e2e 41.94% <100.00%> (+0.08%) ⬆️
unit 57.71% <50.00%> (+0.15%) ⬆️

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.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2024
{"hypen-separated", "hyphenated-name", ""},
{"no install namespace", "", regexMismatchError},
{"dot-separated", "dotted.name", regexMismatchError},
{"includes version", "channel-has-version-1.0.1", regexMismatchError},
Copy link
Contributor

Choose a reason for hiding this comment

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

We already check for dot separated, I don't think we need to explicitly check for "includes version" for install namespaces

Suggested change
{"includes version", "channel-has-version-1.0.1", regexMismatchError},

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep good point, this was a copy/paste job from the channel name tests, and this one does not seem relevant here. I'll remove.

everettraven
everettraven previously approved these changes May 2, 2024
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Looks good. One minor nit, but IMO not something that is worth holding this PR on and will leave up to you if you'd like to address.

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2024
@joelanford joelanford enabled auto-merge May 2, 2024 19:13
@joelanford joelanford added this pull request to the merge queue May 2, 2024
@joelanford joelanford removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 2, 2024
Merged via the queue into operator-framework:main with commit 0f85c25 May 2, 2024
16 of 17 checks passed
@joelanford joelanford deleted the install-namespace branch June 20, 2024 18:59
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.

None yet

3 participants