Skip to content

fix: fix errors during repeated pull and rm operations; add tag validation on pull to ensure idempotency.#484

Open
PikaByter wants to merge 2 commits intomodelpack:mainfrom
PikaByter:main
Open

fix: fix errors during repeated pull and rm operations; add tag validation on pull to ensure idempotency.#484
PikaByter wants to merge 2 commits intomodelpack:mainfrom
PikaByter:main

Conversation

@PikaByter
Copy link
Copy Markdown

@PikaByter PikaByter commented Apr 1, 2026

related issue:#483

FIx:
add tag validation when pulling

Test:

make build
chmod +x ./output/modctl
cd output
./modctl pull <image>
./modctl rmi <image>
./modctl pulll <image>
./modctl rmi <image>

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a StatTag method to the Storage interface and its distribution implementation to verify tag existence. This new method is utilized in the pullIfNotExist function to ensure manifests are pushed when a tag is missing, even if the underlying blob is already present. The changes also include minor formatting adjustments to model file constants and updated mocks. A review comment suggested using errors.As for more robust error handling of wrapped ErrTagUnknown errors.

Comment on lines +293 to +299
if err != nil {
switch err.(type) {
case distribution.ErrTagUnknown:
return false, nil
}
return false, err
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using switch err.(type) to check for distribution.ErrTagUnknown may fail if the error is wrapped. Since the project uses wrapped errors (e.g., %w), it is safer and more idiomatic to use errors.As to check for specific error types.

Suggested change
if err != nil {
switch err.(type) {
case distribution.ErrTagUnknown:
return false, nil
}
return false, err
}
if err != nil {
var tagErr distribution.ErrTagUnknown
if errors.As(err, &tagErr) {
return false, nil
}
return false, err
}

Copy link
Copy Markdown
Contributor

@aftersnow aftersnow left a comment

Choose a reason for hiding this comment

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

(duplicate, please see the English review below)

Copy link
Copy Markdown
Contributor

@aftersnow aftersnow left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! The fix direction is correct. A few suggestions:

Required

  1. Missing digest validation on tag re-creation path: When manifest exists but tag doesn't, io.ReadAll(reader) reads content and pushes it, but the function returns early before validateDigest(). Please add validation or comment why it's safe to skip.

  2. No unit test coverage: The core new path (StatManifest=true, StatTag=false) has no tests. Please add coverage.

Suggestions

  1. The formatting changes in constants.go are unrelated to the fix — consider a separate commit.

  2. The second commit message feat: update dis is unclear — please reword it.

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.

2 participants