⚠️ API updates based on external review #443

everettraven · 2024-10-22T14:43:31Z

No description provided.

api/core/v1alpha1/clustercatalog_types.go

joelanford · 2024-10-28T17:43:49Z

api/core/v1alpha1/clustercatalog_types.go

+// +kubebuilder:validation:XValidation:rule="isURL(self.base)",message="base must be a valid URL"
+// +kubebuilder:validation:XValidation:rule="url(self.base).getScheme() == \"http\" || url(self.base).getScheme == \"https\"",message="scheme must be one of [http, https]"


Can these go on the Base string definition instead of the struct?

Depending on where these validations end up, also be cognizant of the message. For example:

If the validation is on the ClusterCatalogURLs struct, then the message needs to reference the field base by name.

If the validation is on the Base string field, then the apiserver will construct a prefix on our message that includes the base field name. Therefore we don't need to repeat base in the message.

I believe they have to go on the struct for CEL validations.

I will update all the CEL messages to have a reference to the field.

Turns out that you can do this and I totally overlooked that it could. I'm happy to move all CEL validations to the individual fields , where it makes sense, if that is preferred.

api/core/v1alpha1/clustercatalog_types.go

perdasilva · 2024-11-05T12:07:52Z

api/core/v1alpha1/clustercatalog_types.go

 	//
 	// When omitted, the image will not be polled for new content.
-	// +kubebuilder:validation:Format:=duration
+	// +kubebuilder:validation:Minimum:=1


Would we want to start off with a higher minimum? 1 minute feels very frequent.

The higher we go, the longer our operator-controller e2e test for "do we automatically upgrade when a catalog poll finds new content?" will take.

If we can come up with a way to make that test execute faster, I could see increasing this minimum.

codecov · 2024-11-05T17:14:57Z

Codecov Report

Attention: Patch coverage is 82.22222% with 8 lines in your changes missing coverage. Please review.

Project coverage is 38.23%. Comparing base (9cf2e70) to head (6d09548).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
api/core/v1alpha1/zz_generated.deepcopy.go	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #443      +/-   ##
==========================================
+ Coverage   37.83%   38.23%   +0.40%     
==========================================
  Files          15       15              
  Lines        1208     1224      +16     
==========================================
+ Hits          457      468      +11     
- Misses        701      706       +5     
  Partials       50       50

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

api/core/v1alpha1/clustercatalog_types.go

api/core/v1alpha1/clustercatalog_types_test.go

internal/controllers/core/clustercatalog_controller.go

internal/controllers/core/clustercatalog_controller_test.go

internal/source/containers_image.go

LalatenduMohanty · 2024-11-06T15:53:45Z

api/core/v1alpha1/clustercatalog_types.go

+	// These on-cluster components may do a variety of things with this information, such as
+	// presenting the content in a GUI dashboard or installing content from the catalog on the cluster.
+	// The catalog source must contain catalog metadata in the File-Based Catalog (FBC) format.
+	// For more information on FBC, see https://olm.operatorframework.io/docs/reference/file-based-catalogs/#docs.


I hope that we have a way to know when these URL links break.

We don't. I'm not sure it is the end of the world if they do break, but we control that docs site.

api/core/v1alpha1/clustercatalog_types.go

everettraven · 2024-11-06T21:14:52Z

Note: go-apidiff and upgrade-e2e are expected to fail as we've made breaking changes to the API

joelanford · 2024-11-06T21:41:54Z

api/core/v1alpha1/clustercatalog_types.go

+	// base is a cluster-internal URL that provides endpoints for
+	// accessing the content of the catalog.
+	//
+	// It is expected that client append the path for the endpoint they wish


Suggested change

// It is expected that client append the path for the endpoint they wish

// It is expected that clients append the path for the endpoint they wish

joelanford · 2024-11-06T21:42:16Z

api/core/v1alpha1/clustercatalog_types.go

+	// It is expected that client append the path for the endpoint they wish
+	// to access.
+	//
+	// Currently, only a single version of the is served and is accessible at the path


Suggested change

// Currently, only a single version of the is served and is accessible at the path

// Currently, only a single API is served and is accessible at the path

joelanford · 2024-11-06T21:43:10Z

api/core/v1alpha1/clustercatalog_types.go

+	// Currently, only a single version of the is served and is accessible at the path
+	// /api/v1.
+	//
+	// The endpoints served for v1 of the are:


Suggested change

// The endpoints served for v1 of the are:

// The endpoints served for the v1 API are:

Signed-off-by: everettraven <[email protected]> Signed-off-by: Mikalai Radchuk <[email protected]>

m1kola

make generate is missing it seems. Also we need to squash the commits. Other than that - I think it is ready to go.

m1kola

Did make generate and 22 squashed commits into 1.

m1kola · 2024-11-07T10:52:45Z

v0.36.0 includes this PR

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 Oct 22, 2024

grokspawn mentioned this pull request Oct 22, 2024

External API review submission operator-framework/operator-controller#1358

Closed

everettraven commented Oct 28, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Outdated Show resolved Hide resolved

joelanford reviewed Oct 28, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Show resolved Hide resolved

joelanford reviewed Oct 28, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Outdated Show resolved Hide resolved

joelanford reviewed Oct 28, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Outdated Show resolved Hide resolved

joelanford reviewed Oct 28, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Outdated Show resolved Hide resolved

joelanford reviewed Oct 28, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Outdated Show resolved Hide resolved

joelanford reviewed Oct 28, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Outdated Show resolved Hide resolved

perdasilva reviewed Nov 5, 2024

View reviewed changes

LalatenduMohanty mentioned this pull request Nov 5, 2024

catalogd external API review updates #453

Closed

everettraven changed the title ~~wip: first pass at API changes from external review~~ ⚠️ API updates based on external review Nov 5, 2024

joelanford reviewed Nov 6, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Outdated Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Outdated Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Outdated Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Outdated Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Outdated Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Outdated Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Outdated Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types_test.go Outdated Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types_test.go Outdated Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types_test.go Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller.go Outdated Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller_test.go Outdated Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller_test.go Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller_test.go Show resolved Hide resolved

joelanford reviewed Nov 6, 2024

View reviewed changes

internal/source/containers_image.go Outdated Show resolved Hide resolved

everettraven marked this pull request as ready for review November 6, 2024 17:04

everettraven requested a review from a team as a code owner November 6, 2024 17:04

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 Nov 6, 2024

LalatenduMohanty reviewed Nov 6, 2024

View reviewed changes

joelanford previously approved these changes Nov 6, 2024

View reviewed changes

joelanford reviewed Nov 6, 2024

View reviewed changes

everettraven dismissed joelanford’s stale review via 58b72ef November 6, 2024 22:10

joelanford previously approved these changes Nov 6, 2024

View reviewed changes

API updates based on external review

6d09548

Signed-off-by: everettraven <[email protected]> Signed-off-by: Mikalai Radchuk <[email protected]>

m1kola previously approved these changes Nov 7, 2024

View reviewed changes

m1kola dismissed stale reviews from joelanford and themself via 6d09548 November 7, 2024 10:22

m1kola force-pushed the apis/external-review branch from 58b72ef to 6d09548 Compare November 7, 2024 10:22

m1kola approved these changes Nov 7, 2024

View reviewed changes

m1kola added this pull request to the merge queue Nov 7, 2024

Merged via the queue into operator-framework:main with commit 20acf03 Nov 7, 2024
13 of 15 checks passed

m1kola mentioned this pull request Nov 7, 2024

⚠️ Bump catalogd to v0.36.0 operator-framework/operator-controller#1434

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ API updates based on external review #443

⚠️ API updates based on external review #443

everettraven commented Oct 22, 2024

joelanford Oct 28, 2024

joelanford Oct 28, 2024

everettraven Oct 28, 2024

everettraven Oct 28, 2024 •

edited

Loading

everettraven Nov 6, 2024

perdasilva Nov 5, 2024

joelanford Nov 6, 2024

codecov bot commented Nov 5, 2024 •

edited

Loading

LalatenduMohanty Nov 6, 2024

everettraven Nov 6, 2024

everettraven commented Nov 6, 2024

joelanford Nov 6, 2024

joelanford Nov 6, 2024 •

edited

Loading

joelanford Nov 6, 2024

m1kola left a comment

m1kola left a comment

m1kola commented Nov 7, 2024

		// +kubebuilder:validation:XValidation:rule="isURL(self.base)",message="base must be a valid URL"
		// +kubebuilder:validation:XValidation:rule="url(self.base).getScheme() == \"http\" \|\| url(self.base).getScheme == \"https\"",message="scheme must be one of [http, https]"

	// It is expected that client append the path for the endpoint they wish
	// It is expected that clients append the path for the endpoint they wish

	// Currently, only a single version of the is served and is accessible at the path
	// Currently, only a single API is served and is accessible at the path

	// The endpoints served for v1 of the are:
	// The endpoints served for the v1 API are:

⚠️ API updates based on external review #443

⚠️ API updates based on external review #443

Conversation

everettraven commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

everettraven Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 5, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

everettraven commented Nov 6, 2024

Choose a reason for hiding this comment

joelanford Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1kola left a comment

Choose a reason for hiding this comment

m1kola left a comment

Choose a reason for hiding this comment

m1kola commented Nov 7, 2024

everettraven Oct 28, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading

joelanford Nov 6, 2024 •

edited

Loading