Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updata: fix fetching max version in remove-update #362

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ytsssun
Copy link
Contributor

@ytsssun ytsssun commented Jan 24, 2025

Issue number:

Closes # N/A

Description of changes:

  • When running the remove-update command it would report the max version available in the manifest. But it is checking the wrong order so that the latest version is not correctly reported. This is a simple fix for that.

Testing done:
Before the fix:

cargo run --bin updata -- remove-update "${MANIFEST_PATH}/aws-k8s-1.29/x86_64/tuf_in/manifest.json" \
    --arch x86_64\
    --version 1.30.0 \
    --variant aws-k8s-1.29

22:56:06 [INFO] Update x86_64-aws-k8s-1.29-1.30.0 removed. Current maximum version: 1.18.0

After:

cargo run --bin updata -- remove-update "${MANIFEST_PATH}/aws-k8s-1.29/x86_64/tuf_in/manifest.json" \
    --arch x86_64\
    --version 1.30.0 \
    --variant aws-k8s-1.29

22:56:06 [INFO] Update x86_64-aws-k8s-1.29-1.30.0 removed. Current maximum version: 1.31.0

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@@ -118,7 +118,7 @@ impl RemoveUpdateArgs {
});
// Note: We don't revert the maximum version on removal
update_metadata::write_file(&self.file, &manifest)?;
if let Some(current) = manifest.updates.first() {
if let Some(current) = manifest.updates.last() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just not sorted? I would expect to need to sort it to always get the max version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is sorted in a reversed order. I will adjust the code to enforce a sort here so that we get deterministic result.

@bcressey
Copy link
Contributor

Closes # N/A

Can you open an issue with more details about the bug you're fixing?

"max version" is (incorrectly) associated with each update's metadata instead of being set once at the top level of manifest.json. This is a longstanding bug that would need a repo pivot to fix.

In the meantime, tools (pubsys) should all be updating the max version for every entry in the manifest as part of the process of adding a new update entry. If that's not happening, it's potentially a bug in the tool.

Just from a practical perspective: fixing updata is not going to fix the behavior on any deployed systems, which will continue to find the wrong version if the constraint is violated. Better not to violate the constraint than to give up on those systems.

@ytsssun
Copy link
Contributor Author

ytsssun commented Feb 3, 2025

Closes # N/A

Can you open an issue with more details about the bug you're fixing?

"max version" is (incorrectly) associated with each update's metadata instead of being set once at the top level of manifest.json. This is a longstanding bug that would need a repo pivot to fix.

Just created the issue - #366. Right now updata is not even referencing the max_version field in manifest.json. I can confirm that all entires in the updates in manifest.json does have the correct max_version set.

Now I think the simple fix would probably be to just use max_version instead of version to get the max version. Or we can sort the updates array by version and get the max version with the sorted array.

- When running the remove-update command it would report the max version
available in the manifest. But it is checking the wrong order so that
the latest version is not correctly reported. This is a simple fix for
that. Instead of reading the version, we read the max_version field
so that the max version is correctly captured.

Signed-off-by: Yutong Sun <[email protected]>
@ytsssun ytsssun force-pushed the fix-updata-max-version branch from 6662d30 to dc50ff0 Compare February 4, 2025 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants