direct: ignore remote-only changes on name-based ID fields#5599
Open
denik wants to merge 11 commits into
Open
Conversation
Contributor
Approval status: pending
|
denik
force-pushed
the
denik/named-id-fields
branch
from
de92903 to
380841e
Compare
Collaborator
Integration test reportCommit: 66684c6
22 interesting tests: 15 SKIP, 7 KNOWN
Top 24 slowest tests (at least 2 minutes):
|
denik
force-pushed
the
denik/named-id-fields
branch
from
4f7b7ab to
b99237b
Compare
Resources fetched by a name-based ID (schemas, volumes, registered models, apps, secret scopes, postgres resources, etc.) cannot exhibit real remote drift on the fields composing that ID: a successful get-by-ID means a differing remote value can only be backend normalization (e.g. UC lowercasing), since an out-of-band rename would 404 and is already handled as resource-gone. Reacting with recreate or rename is therefore never correct for remote-only diffs on these fields. Encode this declaratively: - named_id_fields: ID components; local changes recreate, remote-only diffs are skipped. Replaces their recreate_on_changes entries. - update_id_on_local_changes (renamed from update_id_on_changes): same skip for remote-only diffs; local changes still rename via DoUpdateWithID. normalize_case is unchanged: it covers local case-only edits, which the get-by-ID argument does not, and which would otherwise recreate a resource UC considers unchanged. Co-authored-by: Isaac
Revert the cosmetic update_id_on_changes -> update_id_on_local_changes rename to minimize the diff (catalogs, external_locations, volumes, secret_scopes.permissions keep their existing key). The semantics shift is explained in comments instead: update_id_on_changes now only governs local changes, since remote-only diffs on these ID fields are skipped by shouldSkipIDField. Co-authored-by: Isaac
The fields hold a user-provided name that forms the resource ID (as opposed to a server-generated id); "provided" makes clear why a remote-only difference can only be backend normalization. Source-only rename; reason strings (id_field) are unchanged. Also regenerate volumes/uppercase-name/out.deploy.direct.txt, whose id_field reasons were reverted to the stale uc_case by the rebase auto-merge. Co-authored-by: Isaac
provided_id_fields already skips the remote-only diffs that normalize_case was added for (the no-op-redeploy bug), so normalize_case only guarded a local case-only edit — an untested corner, gated by the destructive-action prompt for schemas/volumes, and inconsistent with the other provided_id_fields resources that never had it. Remove the field, its shouldSkipNormalized branch, and the schemas/volumes yaml blocks. normalize_slash is unaffected. Also regenerate model_serving_endpoints/basic/out.second-plan.direct.json (immutable -> id_field), a cloud-only golden carrying the reason change from moving name to provided_id_fields; local task test skips it, so CI integration caught it. Co-authored-by: Isaac
The method now also covers provided_id_fields (both categories recreate on a local change), so the recreate_on_changes-specific name was inaccurate. Co-authored-by: Isaac
The provided_id_fields / update_id_on_changes logic was split across shouldSkipIDField (remote-only -> skip) and shouldUpdateOrRecreate (local -> recreate/rename), so correctness depended on the first running before the second in the ladder. Fold both into classifyIDField, which decides skip vs recreate/UpdateWithID in one place from Old==New. The leftover shouldUpdateOrRecreate then only matched recreate_on_changes, so inline it as a direct findMatchingRule call (cfg is never nil here). Pure refactor; no behavior or golden changes. Co-authored-by: Isaac
denik
force-pushed
the
denik/named-id-fields
branch
from
f99e1b4 to
042a58e
Compare
Co-authored-by: Isaac
recreate_on_changes, provided_id_fields, and update_id_on_changes each decide a field's action, and classifyIDField runs before recreate_on_changes in the ladder, so a field in more than one would have dead entries and the categories disagree on remote-only diffs. Add a test that a field appears in at most one. Co-authored-by: Isaac
Pairs with provided_id_fields: both compose the resource's user-provided ID and are handled by classifyIDField (remote-only diffs skipped), differing only in the local-change action — provided_id_fields recreates, updatable_id_fields renames via UpdateWithID. The *_id_fields naming makes the family explicit and is more accurate than "update_id_on_changes", which post-refactor also governs the remote-only skip. Source-only; the per-field reason (id_changes) is unchanged so no golden churn. Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #5531 we fixed spurious recreation of schemas/volumes due to backend lowercasing fields that are part of the id/name. This was fixed by making comparison for those fields case-insensitive.
This is more general fix for all resources that have ID fields configured in bundle config based on the following: if we can fetch the resource by ID, then even if fields come back changed, we should not recreate, because change can only be attributed to normalization, not replacement.