Skip to content

🌱 use "preflight" in ClusterObjectSet rejection messages; add static validation e2e - #3026

Merged
openshift-merge-bot[bot] merged 2 commits into
operator-framework:mainfrom
perdasilva:cos-static-validation-e2e
Oct 19, 2026
Merged

openshift-merge-bot[bot] merged 2 commits into
operator-framework:mainfrom
perdasilva:cos-static-validation-e2e

Conversation

@perdasilva

@perdasilva perdasilva commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Description

Two related changes for the ClusterObjectSet PreconditionsNotMet pre-apply checks:

1. Reword the condition messages: "pre-apply check" → "preflight check"

The controller's revision- and phase-level pre-apply rejection messages now say "A preflight check rejected the revision/phase: …". "Preflight" is the term users already recognize for checks that run before an operation (kubeadm, Helm, etc.), and it's the vocabulary already used in the controller's own comments, the Ready condition documentation in clusterobjectset_types.go, and the concepts docs. The condition reason (PreconditionsNotMet) is unchanged — only the human-readable message text.

Also aligned the explanatory comments in the ClusterExtension controller that describe this reason.

No CRD/reference-doc regeneration was needed: the Ready condition docstring and docs/draft/concepts/clusterobjectsets.md already used "preflight".

2. Add cos_static_validation.feature

A new e2e suite covering the revision-level (static, non-dry-run) preflight checks, which surface as Ready=False / PreconditionsNotMet:

  1. Duplicate object across phases → A preflight check rejected the revision: … duplicate object found in phases: phase-a, phase-b
  2. Forbidden metadata (ownerReferences baked into a manifest) → A preflight check rejected the revision: … metadata.ownerReferences: Forbidden: must be empty.

Testing

  • Unit tests for the two changed controller packages pass.
  • The e2e suite passes against the object-controller built from this branch (rebuilt with the new message):
2 scenarios (2 passed)
8 steps (8 passed)

Notes

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Updates
    • Validation failure messages now describe failed checks as “preflight checks” instead of “pre-apply checks.” Failure reasons and retry behavior are unchanged.
  • Tests
    • Added end-to-end coverage for validation failures when the same ConfigMap appears in multiple phases or a ConfigMap manifest contains owner references. The scenarios verify that the affected revision and validation failure are reported.

@netlify

netlify Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 0caedfc
🔍 Latest deploy log https://app.netlify.com/projects/olmv1/deploys/6ad6309027f249168fba0a22
😎 Deploy Preview https://deploy-preview-3026--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.

@perdasilva
perdasilva force-pushed the cos-static-validation-e2e branch 2 times, most recently from cd17098 to 09abda9 Compare October 19, 2026 09:47
@perdasilva perdasilva changed the title 🌱 add ClusterObjectSet static pre-apply validation e2e scenarios 🌱 use "preflight" in ClusterObjectSet rejection messages; add static validation e2e Oct 19, 2026
@perdasilva

Copy link
Copy Markdown
Contributor Author

/hold until group PR merges

@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 Oct 19, 2026
@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 20552860-4a08-4775-af53-00ef0b4a5ab3
📥 Commits

Reviewing files that changed from the base of the PR and between 09abda9 and 0caedfc.

📒 Files selected for processing: 1
  • test/e2e/features/cos_static_validation.feature

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.

📝 Walkthrough

Controller validation messages and comments now use “preflight” instead of “pre-apply.” New end-to-end scenarios check PreconditionsNotMet for duplicate objects across phases and manifests with metadata.ownerReferences.

Changes: ClusterObjectSet preflight validation

Layer / File(s) Summary
Validation outcomes and reporting
internal/object-controller/controllers/clusterobjectset_controller.go, internal/operator-controller/controllers/common_controller.go, test/e2e/features/cos_static_validation.feature
Controller messages and comments use “preflight” terminology. End-to-end scenarios assert PreconditionsNotMet for a duplicate object across phases and a manifest with metadata.ownerReferences.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to 0caed

No actionable issue is identified that should hold this change beyond normal checks.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title is concise, specific, uses the required seedling prefix, and accurately summarizes the message update and new static-validation E2E coverage.
Description check Passed The description clearly explains the message changes, unchanged reason, new E2E scenarios, test results, scope, and follow-up PR. The reviewer checklist from the template is not included, but the core…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 …
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description clearly explains the message changes, unchanged reason, new E2E scenarios, test results, scope, and follow-up PR. The reviewer checklist from the template is not included, but the core description is complete.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Per G. da Silva and others added 2 commits October 19, 2026 16:45
Use "preflight" instead of "pre-apply" in the PreconditionsNotMet Ready
messages for the ClusterObjectSet revision- and phase-level checks that
run before objects are applied. "Preflight" is the term users already
recognize for checks that run before an operation, and it matches the
vocabulary already used in the controller comments, the Ready condition
documentation, and the concepts docs. The condition reason
(PreconditionsNotMet) is unchanged.

Also align the explanatory comments in the ClusterExtension controller
that describe this reason.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Per G. da Silva <pegoncal@redhat.com>
Add a cos_static_validation.feature suite covering the revision-level
(static, non-dry-run) preflight checks, which surface on the
ClusterObjectSet as Ready=False/PreconditionsNotMet:

- the same object (GVK + namespace + name) appearing in more than one
  phase, and
- an object manifest that carries its own metadata.ownerReferences.

Both assert the controller's "A preflight check rejected the revision:
..." message.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Per G. da Silva <pegoncal@redhat.com>
@perdasilva
perdasilva force-pushed the cos-static-validation-e2e branch from 09abda9 to 0caedfc Compare October 19, 2026 15:00
@perdasilva

Copy link
Copy Markdown
Contributor Author

/unhold

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 19, 2026

@fao89 fao89 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.

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2026

@fgiudici fgiudici left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@tmshort

tmshort commented Oct 19, 2026

Copy link
Copy Markdown
Member

/approve

@openshift-ci

openshift-ci Bot commented Oct 19, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tmshort

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 Oct 19, 2026
@openshift-merge-bot
openshift-merge-bot Bot merged commit 01d8b30 into operator-framework:main Oct 19, 2026
46 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants