[RaphaelFakhri]

Description of your changes

config.MoveToStatus returns as soon as one of the supplied field paths does not resolve to a schema. Every field path listed after the missing one is then silently left in spec. For example, config.MoveToStatus(r.TerraformResource, "removed_attr", "route") keeps route as a configurable parameter, and the generated CRD has no error or warning to show it. This can happen when a Terraform provider upgrade drops or renames an attribute that a resource configuration still lists.

This change makes the loop continue to the next field path instead, which matches how MarkAsRequired already treats a field path it cannot find. A field path that resolves is handled exactly as before.

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 the DoesNotExistFollowedByExisting case to TestMoveToStatus. It passes a missing field path followed by an existing one and expects the existing field to be moved to status.

• Without the fix, go test ./pkg/config -run TestMoveToStatus fails on the new case: topA stays Optional: true, Computed: false.
• With the fix, all four TestMoveToStatus cases pass, and go test ./pkg/config/... passes.

Fixes #804

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

MoveToStatus now continues processing field paths after one path has no matching schema. A test verifies that a later existing field is still moved to computed.

Changes: MoveToStatus field paths

Layer / File(s)	Summary
Continue processing field paths pkg/config/common.go, pkg/config/common_test.go	MoveToStatus continues when a field path has no matching schema. The test checks that a later existing field becomes computed while another field remains optional.

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

Change: Bug fix

Suggested reviewers: ulucinar

Merge Risk: Minimal · up to 34adf

The change allows later valid field paths to be processed after a missing path. No merge-blocking concern is evident from the supplied context; merge after normal checks pass.

Pre-merge checks: 7

Check name	Status	Explanation
Title check		The title is 50 characters, under the 72-character limit, and clearly describes the change to continue past missing field paths in MoveToStatus.
Description check		The description directly explains the MoveToStatus bug, the fix, the regression test, and the reported test results.
Linked Issues check		Thank you for the focused fix. Issue #804 has one coding requirement: MoveToStatus must keep processing field paths after one does not resolve to a schema. The diff at pkg/config/common.go changes the…
Out of Scope Changes check		The whole-PR diff against the merge base ff4a298 changes two files. pkg/config/common.go has a one-line change from return to continue in MoveToStatus. pkg/config/common_test.go adds one table-dri…
Configuration Api Breaking Changes		The pull request changes only the MoveToStatus control flow from return to continue and adds a test case. Its exported signature remains `MoveToStatus(sch *schema.Resource, fieldpaths ...string)…
Generated Code Manual Edits		The pull request changes only pkg/config/common.go and pkg/config/common_test.go. No changed file matches the zz_*.go pattern, so the manual generated-code edit condition does not apply.
Template Breaking Changes		The pull request changes only pkg/config/common.go and pkg/config/common_test.go. The authoritative diff contains no changes to pkg/controller/external*.go templates, so it cannot introduce the …

Full details: Linked Issues check

Explanation

Thank you for the focused fix. Issue #804 has one coding requirement: MoveToStatus must keep processing field paths after one does not resolve to a schema. The diff at pkg/config/common.go changes the nil-schema branch from return to continue. The loop now moves each later field path. The reproduction in #804 calls MoveToStatus(sch, "doesNotExist", "topA"). With this change, topA becomes Optional: false and Computed: true, as the issue expects. The change matches the MarkAsRequired behavior that #804 cites. The new DoesNotExistFollowedByExisting case in pkg/config/common_test.go covers the same sequence (topD missing, then topA). It expects topA to become Computed: true and topB to stay Optional: true. Resolvable paths follow the same code path as before. The nested recursive call and the rest of the function are unchanged. I did not run the tests. The PASS rests on the code reading and the author's reported test results. The PR does not carry any non-coding task from #804.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff against the merge base ff4a298 changes two files. pkg/config/common.go has a one-line change from return to continue in MoveToStatus. pkg/config/common_test.go adds one table-driven test case to TestMoveToStatus. Both changes serve the objective in #804. No unrelated files or refactors appear in the diff. The PR description says the change applies only to missing paths in MoveToStatus, and the diff matches that statement.

Full details: Configuration Api Breaking Changes

Explanation

The pull request changes only the MoveToStatus control flow from return to continue and adds a test case. Its exported signature remains MoveToStatus(sch *schema.Resource, fieldpaths ...string). No public type, function, or field is removed or renamed, and no existing function gains required parameters. The breaking-change label is not required.

Full details: Template Breaking Changes

Explanation

The pull request changes only pkg/config/common.go and pkg/config/common_test.go. The authoritative diff contains no changes to pkg/controller/external*.go templates, so it cannot introduce the specified template breaking change.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

ast-grep (0.51.1)

ast-grep could not parse rule config: /ast-grep-rules/ast-grep-essentials/rules/javascript/security/insecure-cookie.yml

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

• X
• Mastodon
• Reddit
• LinkedIn

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

Recent review info

Run configuration

• Source mode: Auto
• Repository: crossplane/upjet
• Branch: main
• File: .coderabbit.yaml
• Commit: ff4a298
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 3b1b655b-901a-4e60-ba89-9b2b66d66b4a

Commits

Reviewing files that changed from the base of the PR and between ff4a298 and 34adf51.

Files selected for processing: 2

• pkg/config/common.go
• pkg/config/common_test.go

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

[blacksoxx]

Reads right to me. The return on a missing path silently dropped every path listed after it, and MarkAsRequired already skips a missing path and carries on, so the two helpers now behave the same way. The DoesNotExistFollowedByExisting case pins the order that mattered. Read against head 34adf51 on main ff4a298, not run locally.

I would approve this; the decision is with the maintainers.