Skip to content

fix(types): omit XValidation for required fields with references - #684

Merged
ulucinar merged 1 commit into
crossplane:mainfrom
jonasz-lasut:mark-as-required
Jun 29, 2026
Merged

fix(types): omit XValidation for required fields with references#684
ulucinar merged 1 commit into
crossplane:mainfrom
jonasz-lasut:mark-as-required

Conversation

@jonasz-lasut

Copy link
Copy Markdown
Collaborator

Description of your changes

When a field is marked as required but also has a reference configured, the field becomes satisfiable via *Ref/*Selector mechanisms. Explicitly set f.Required=false in NewReferenceField so no XValidation rule is emitted for such fields.

Fixes issue observed in crossplane-contrib/provider-upjet-gcp#952

I have:

  • Read and followed Upjet's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
    - [ ] Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Added unit test that validates this behavior

When a field is marked as required but also has a reference configured,
the field becomes satisfiable via *Ref/*Selector mechanisms. Explicitly
set f.Required=false in NewReferenceField so no XValidation rule is
emitted for such fields.

Signed-off-by: Jonasz Łasut-Balcerzak <jonasz@upbound.io>
@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1facd6c5-d79b-4c59-8efc-feb383fac6a1

📥 Commits

Reviewing files that changed from the base of the PR and between 993d58a and 3b9b01c.

📒 Files selected for processing (2)
  • pkg/types/builder_test.go
  • pkg/types/field.go

📝 Walkthrough

Walkthrough

Referenced fields now clear their required state, and the build test adds a regression case confirming that a required referenced field does not emit XValidation.

Changes

Reference field required handling

Layer / File(s) Summary
Reference field required flag
pkg/types/field.go
NewReferenceField sets f.Required = false for referenced fields.
Build regression test
pkg/types/builder_test.go
TestBuild adds a per-case setup hook and a regression case covering a required referenced field with no XValidation.

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 7
✅ Passed checks (7 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the core change: omitting XValidation for required fields that also have references.
Description check ✅ Passed The description matches the code changes and explains the reference-driven Required=false behavior and test coverage.
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.
Configuration Api Breaking Changes ✅ Passed PASS: The PR only changes pkg/types files; no pkg/config/** files or public signatures were touched.
Generated Code Manual Edits ✅ Passed No changed file matches zz_*.go; the PR only touches pkg/types/field.go and pkg/types/builder_test.go, so no generated-code manual edit is present.
Template Breaking Changes ✅ Passed Only pkg/types/field.go and its test were changed; no pkg/controller/external*.go templates were modified.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@ulucinar ulucinar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you @jonasz-lasut, lgtm.

Comment thread pkg/types/field.go
@ulucinar
ulucinar merged commit 2100488 into crossplane:main Jun 29, 2026
9 checks passed
@github-actions

Copy link
Copy Markdown

Backport failed for release-1.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-1.11
git worktree add -d .worktree/backport-684-to-release-1.11 origin/release-1.11
cd .worktree/backport-684-to-release-1.11
git switch --create backport-684-to-release-1.11
git cherry-pick -x 3b9b01c64126c413d156c8867a2b0da399a5064b

@github-actions

Copy link
Copy Markdown

Backport failed for release-1.10, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-1.10
git worktree add -d .worktree/backport-684-to-release-1.10 origin/release-1.10
cd .worktree/backport-684-to-release-1.10
git switch --create backport-684-to-release-1.10
git cherry-pick -x 3b9b01c64126c413d156c8867a2b0da399a5064b

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants