Skip to content

✨ ClusterObjectSet/Deployment API review fixes - #3128

Open
dtfranz wants to merge 1 commit into
operator-framework:mainfrom
dtfranz:cos-api-review-remainder
Open

dtfranz wants to merge 1 commit into
operator-framework:mainfrom
dtfranz:cos-api-review-remainder

Conversation

@dtfranz

@dtfranz dtfranz commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Require spec.phases, tighten field validations (DNS/secret-key formats, object name path-segment rule), lower the ClusterObjectDeployment progress deadline minimum to 1, move enum markers onto their named types, and update tests, generated manifests, and e2e features accordingly.

API Lint Diff Results
=====================
Baseline (main): 39 issues
Current branch: 33 issues

FIXED: 7
NEW: 1
PRE-EXISTING: 32

=== FIXED ISSUES ===
api/v1/clusterobjectdeployment_types.go:80:2:kubeapilinter:optionalfields: field ClusterObjectDeploymentTemplate.Metadata has a valid zero value ({}), but the validation is not complete (e.g. min properties/adding required fields). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer.
api/v1/clusterobjectset_types.go:469:2:kubeapilinter:optionalfields: field ObjectSourceRef.Namespace has a valid zero value (""), but the validation is not complete (e.g. minimum length). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer.
api/v1/clusterobjectset_types.go:557:2:kubeapilinter:optionalfields: field ClusterObjectSetStatus.CompletedAt should have the omitempty tag.
api/v1/clusterobjectset_types.go:582:2:kubeapilinter:optionalfields: field ObservedPhase.CompletedAt should have the omitempty tag.
api/v1/clusterobjectset_types.go:611:2:kubeapilinter:requiredfields: field ObservedPhase.LastTransitionTime should have the omitempty tag.
api/v1/clusterobjectset_types.go:665:2:kubeapilinter:optionalfields: field ObjectStatus.Group has a valid zero value (""), but the validation is not complete (e.g. minimum length). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer.
api/v1/clusterobjectset_types.go:685:2:kubeapilinter:optionalfields: field ObjectStatus.Namespace has a valid zero value (""), but the validation is not complete (e.g. minimum length). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer.

=== NEW ISSUES ===
api/v1/clusterobjectset_types.go:821:2:kubeapilinter:optionalfields: field ClusterObjectSet.Status has a valid zero value ({"objectCounts": {}}), but the validation is not complete (e.g. min properties/adding required fields). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer.

This PR does introduce another issue with the ClusterObjectSet.Status - not sure what the best path forward with that one is.

Description

Reviewer Checklist

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

Require spec.phases, tighten field validations (DNS/secret-key formats,
object name path-segment rule), lower the ClusterObjectDeployment progress
deadline minimum to 1, move enum markers onto their named types, and update
tests, generated manifests, and e2e features accordingly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Daniel Franz <dfranz@redhat.com>
@netlify

netlify Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit c7b0390
🔍 Latest deploy log https://app.netlify.com/projects/olmv1/deploys/6aedd20d891864984c4931d8
😎 Deploy Preview https://deploy-preview-3128--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai

coderabbitai Bot commented Nov 6, 2026

Copy link
Copy Markdown

Warning

Review limit reached

This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry.

Next included review available in 31 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ebb15394-b21a-4058-8c21-397794ef09af
Commits

Reviewing files that changed from the base of the PR and between 0fcb19e and c7b0390.

Files selected for processing: 20

  • api/v1/clusterobjectdeployment_types.go
  • api/v1/clusterobjectset_types.go
  • api/v1/clusterobjectset_types_test.go
  • api/v1/validation_test.go
  • applyconfigurations/api/v1/clusterobjectdeploymenttemplatespec.go
  • applyconfigurations/api/v1/clusterobjectsetphase.go
  • applyconfigurations/api/v1/objectselector.go
  • applyconfigurations/api/v1/observedphase.go
  • applyconfigurations/api/v1/progressionprobe.go
  • docs/api-reference/olmv1-api-reference.md
  • hack/api-lint-diff/run.sh
  • helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectdeployments.yaml
  • helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml
  • internal/operator-controller/controllers/boxcutter_reconcile_steps_test.go
  • manifests/experimental-e2e.yaml
  • manifests/experimental.yaml
  • test/e2e/features/cod_progressing_reasons.feature
  • test/e2e/features/cos_revision_identity.feature
  • test/e2e/features/group.feature
  • test/e2e/features/install.feature
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

@perdasilva

Copy link
Copy Markdown
Contributor

overriding the api lint failure:

Neither "pure" fix fits this particular field:

  • Pointer is undesirable by design. The field's own doc says: "Cannot be removed once set; this is to prevent immutable fields such as completedAt from being cleared by an update which omits the status." A *ClusterObjectSetStatus reintroduces exactly the clear-to-nil behavior they're avoiding.

  • MinProperties on Status is wrong too, because an empty status is a legitimate state (before the first reconcile writes anything).

@perdasilva

Copy link
Copy Markdown
Contributor

/approve

@openshift-ci

openshift-ci Bot commented Nov 6, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: perdasilva

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-diff-lint-override approved Indicates a PR has been approved by an approver from all required OWNERS files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants