Skip to content

⚠ Implement API changes according to RFC spec - #1166

Merged
everettraven merged 1 commit into
operator-framework:mainfrom
jsm84:unionized
Aug 29, 2024
Merged

⚠ Implement API changes according to RFC spec#1166
everettraven merged 1 commit into
operator-framework:mainfrom
jsm84:unionized

Conversation

@jsm84

@jsm84jsm84 commented Aug 22, 2024

Copy link
Copy Markdown
Contributor

resolves#1088

Summary:

  • v1 API now uses a SourceConfig discriminated union which will allow modularity for future install sources (bundles, charts, etc).
  • SourceConfig uses CEL validation to ensure only valid field names & values are utilized (sourceType: Catalog ensures that the catalog field is also set in SourceConfig).
  • Added new clusterextension_admission unit test for SourceConfig objects. The test covers both valid and invalid cases.
  • Fixed clusterextension_controller test where an unset ClusterExtension spec caused a null pointer deref.
  • Moved ClusterSelector from ClusterExtension.Spec to ClusterExtension.Source.Catalog
    and renamed to Selector.
  • Updated GoDocs to reflect the new API spec and included post-review changes
  • Fixed all definitions of kind: ClusterExtension in docs and scripts
    to reflect the API changes.

@jsm84
jsm84 requested a review from a team as a code ownerAugust 22, 2024 20:55
@netlify

netlifyBot commented Aug 22, 2024

Copy link
Copy Markdown

Deploy Preview for olmv1 ready!

NameLink
🔨 Latest commitf943ddb
🔍 Latest deploy loghttps://app.netlify.com/sites/olmv1/deploys/66d0973dda051f0008b860e7
😎 Deploy Previewhttps://deploy-preview-1166--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.

Comment threadapi/v1alpha1/clusterextension_types.go Outdated
Comment threadapi/v1alpha1/clusterextension_types.go Outdated
Comment threadapi/v1alpha1/clusterextension_types.go Outdated
Comment threadapi/v1alpha1/clusterextension_types.go Outdated
Comment threadapi/v1alpha1/clusterextension_types.go Outdated
Comment threadapi/v1alpha1/clusterextension_types.go Outdated
Comment threadapi/v1alpha1/clusterextension_types.go Outdated
Comment threadapi/v1alpha1/clusterextension_types.go Outdated

@everettraveneverettraven left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this PR @jsm84 !

Requesting changes on my current comments. I'll be out tomorrow, so I'll circle around for further review on Monday.

@codecov

codecovBot commented Aug 22, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 57.14286% with 12 lines in your changes missing coverage. Please review.

Project coverage is 77.18%. Comparing base (bf7f766) to head (8e55961).
Report is 1 commits behind head on main.

Files with missing linesPatch %Lines
api/v1alpha1/zz_generated.deepcopy.go45.45%12 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #1166 +/- ##
==========================================
- Coverage 77.55% 77.18% -0.38% 
==========================================
Files 35 35 Lines 1916 1937 +21 ==========================================
+ Hits 1486 1495 +9 - Misses 296 308 +12 
Partials 134 134 
FlagCoverage Δ
e2e58.59% <53.57%> (-0.18%)⬇️
unit54.67% <21.42%> (-0.60%)⬇️

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.

Comment threadapi/v1alpha1/clusterextension_types.go Outdated
@everettraven

Copy link
Copy Markdown
Contributor

Just finished combing through the rest of the PR and I don't have any additional comments

@jsm84
jsm84force-pushed the unionized branch 2 times, most recently from a991906 to 00528fcCompareAugust 27, 2024 01:08

@everettraveneverettraven left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some more comments on the comments. Overall I think they are good.

Comment threadapi/v1alpha1/clusterextension_types.go Outdated
Comment threadapi/v1alpha1/clusterextension_types.go Outdated
Comment threadapi/v1alpha1/clusterextension_types.go Outdated
Comment threadapi/v1alpha1/clusterextension_types.go Outdated
Comment threadapi/v1alpha1/clusterextension_types.go Outdated
Comment threadapi/v1alpha1/clusterextension_types.go Outdated
@jsm84
jsm84force-pushed the unionized branch 2 times, most recently from f188dd1 to fe81727CompareAugust 28, 2024 18:01
everettraven
everettraven previously approved these changes Aug 28, 2024

@everettraveneverettraven left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes looks good to me, thanks for the contribution @jsm84 !

I finished my review on mobile and it doesn't look like I can kick CI via the GH mobile app. I'll kick it first thing tomorrow morning and try to get this merged.

@everettraven
everettraven added this pull request to the merge queue Aug 29, 2024
@github-merge-queue
github-merge-queueBot removed this pull request from the merge queue due to failed status checks Aug 29, 2024
@everettraven
everettraven added this pull request to the merge queue Aug 29, 2024
@everettraven
everettraven removed this pull request from the merge queue due to a manual request Aug 29, 2024
@everettraven

Copy link
Copy Markdown
Contributor

Closing and re-opening to try and reset the netlify checks that seem to be stuck

auto-merge was automatically disabled August 29, 2024 17:25

Pull request was closed

resolvesoperator-framework#1088
Summary:
* v1 API now uses a `SourceConfig` discriminated union which will allow
modularity for future install sources (bundles, charts, etc).
* `SourceConfig` uses CEL validation to ensure only valid field names &
values are utilized (`sourceType: Catalog` ensures that the `catalog`
field is also set in `SourceConfig`).
* Added new `clusterextension_admission` unit test for `SourceConfig`
objects. The test covers both valid and invalid cases.
* Fixed `clusterextension_controller` test where an unset
`ClusterExtension` spec caused a null pointer deref.
* Moved `ClusterSelector` from `ClusterExtension.Spec` to `ClusterExtension.Source.Catalog`
and renamed to `Selector`.
* Updated GoDocs to reflect the new API spec and included post-review changes
* Fixed all definitions of `kind: ClusterExtension` in docs and scripts
to reflect the API changes.
Signed-off-by: Josh Manning <19478595+jsm84@users.noreply.github.com>
@everettraven
everettraven added this pull request to the merge queue Aug 29, 2024
Merged via the queue into operator-framework:main with commit fc888e5Aug 29, 2024
@skattojuskattoju mentioned this pull request Sep 25, 2024
4 tasks
Sign up for freeto 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.

api: move catalog sourcing fields to discriminated union

2 participants

@jsm84@everettraven