fix(sync): restore the empty params table for a Build image - #1584
Open
cplieger wants to merge 2 commits into
Open
fix(sync): restore the empty params table for a Build image#1584cplieger wants to merge 2 commits into
cplieger wants to merge 2 commits into
Conversation
A Deployment with a Build image whose build is unset serializes to TOML that
cannot be read back, so every later read of the sync file fails and the sync
stops until someone edits the file.
Deployment::edit_config_object renames build_id to 'build' and REMOVES
'version' when it is 0.0.0, which means latest. When the build is also unset
that leaves no keys at all, skip_empty_object drops the whole params table, and
the emitted
image.type = "Build"
cannot be deserialized, because DeploymentImage is adjacently tagged
(tag = "type", content = "params").
This is the same hazard the Builder impl in this file already works around one
level up, appending 'params = {}' because 'toml_pretty will remove empty map
but in this case its needed to deserialize the enums', and the same one the
Procedure stage serializer avoids with skip_empty_object(false) because 'if the
execution.params are fully missing, deserialization will fail'. The nested
image case was missed.
Tests cover all three states: the empty table is dropped and fails to parse,
restoring it round trips, and a populated image is unaffected.
ReplaceIds rewrites image.build_id to the Build name for the sync TOML and fell back to an empty string when the lookup missed. That overwrites a reference the user committed to a git-tracked file with nothing, and it is the most common way to reach the empty params table the previous commit now guards against. The lookup misses whenever the reference is dangling, and a dangling reference is the normal outcome here: deleting a resource clears permissions, recently-viewed and alerter targets via delete_from_alerters, but nothing clears a Deployment's build_id. Build::pre_delete is empty and post_delete only drops a build_state_cache entry. Preserving the value on a miss keeps the declared reference visible instead of erasing it, and matches the same change made for ResourceTarget ids in moghtech#1581. It does not make the reference resolve; a dangling build id stays dangling, and is now legible rather than silently blank.
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
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.
A Deployment with a Build image whose build is unset serializes to TOML that Komodo cannot read back, so every later read of the sync file fails. Same failure class as #1581, reached without any name/id confusion.
The bug
Deployment::edit_config_objectrenamesbuild_idtobuildand removesversionwhen it is 0.0.0, which the entity documents as meaninglatest. If the build is also unset, no keys remain,skip_empty_objectdrops the wholeparamstable, andDeploymentImageis adjacently tagged (tag = "type", content = "params"), so this cannot be deserialized:Tests in this PR cover all three states: the empty table is dropped and fails to parse, restoring it round trips, and a populated image is unaffected.
Precedent
This file already works around the identical hazard twice, both with comments:
Builder::push_additionalappends a literalparams = {}because "toml_pretty will remove empty map but in this case its needed to deserialize the enums".TOML_PRETTY_OPTIONS.skip_empty_object(false)because "If the execution.params are fully missing, deserialization will fail".The nested image case was missed. The first commit follows the Builder precedent, one level deeper:
image.params = {}.Two ways to reach it
Build unset.
validate_configdoes not require a non-emptybuild_idfor a Build image, so a Deployment in that state is constructible and its export is already unreadable.A dangling reference.
ReplaceIdsblankedbuild_idwhen the Build lookup missed, which is the common route into the state above. The lookup misses whenever the reference is dangling, and a dangling reference is the normal outcome here: deleting a resource clears permissions, recently-viewed and alerter targets viadelete_from_alerters, but nothing clears a Deployment'sbuild_id.Build::pre_deleteis empty andpost_deleteonly drops abuild_state_cacheentry.The second commit stops that blanking. It is not the parse fix, and I have kept it separate on purpose: its value is that it no longer overwrites a reference the user committed to a git-tracked file with an empty string, matching the change made for
ResourceTargetids in #1581. It does not make a dangling reference resolve; it makes it legible instead of blank. Drop that commit if you would rather keep blanking.Deliberately not included
The sibling lookups in
ReplaceIds(server_id,swarm_id,linked_repo,builder_id, and the twoserver_idsvectors) blank the same way. They do not break parsing, because those fields carry#[serde(default)]and an empty string is valid TOML, so I have left them alone rather than send a dozen unrequested edits. They do still erase a committed reference. Happy to follow up if you want them consistent.Verification
cargo check -p komodo_coreclean,cargo fmt --checkclean, the three new tests pass.bin/corehad no test module before this, so tell me if you would rather these lived elsewhere or not at all.