Skip to content

image/tarexport: remove suport for loading v0/v1 images#50324

Merged
cpuguy83 merged 3 commits intomoby:masterfrom
thaJeztah:rm_legacy_load
Jul 4, 2025
Merged

image/tarexport: remove suport for loading v0/v1 images#50324
cpuguy83 merged 3 commits intomoby:masterfrom
thaJeztah:rm_legacy_load

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 3, 2025

image/tarexport: remove suport for loading v0/v1 images

This removes the tarexporter.legacyLoadImage method and related helpers.
This functionality was added in 01ba0a9
(docker v1.10), which introduced the new content-addressable image
format; this code provided backward-compatibility with older archives
which contained v0/v1 images.

image/tarexport: inline validateManifest utility

It was just checking if a value is nil; no need to maintain a utility
for that.

- Human readable description for the release notes

Remove support for loading legacy (pre Docker 1.10) images.

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah added this to the 29.0.0 milestone Jul 3, 2025
@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 3, 2025

One test looks to be depending on this;

=== FAIL: amd64.integration-cli TestDockerCLISaveLoadSuite/TestLoadZeroSizeLayer (0.05s)
    docker_cli_save_load_test.go:242: assertion failed: 
        Command:  /usr/local/cli-integration/docker load -i testdata/emptyLayer.tar
        ExitCode: 1
        Error:    exit status 1
        Stdout:   
        Stderr:   open /var/lib/docker/tmp/docker-import-268866837/manifest.json: no such file or directory
        
        
        Failures:
        ExitCode was 1 expected 0
        Expected no error
    --- FAIL: TestDockerCLISaveLoadSuite/TestLoadZeroSizeLayer (0.05s)

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 4, 2025 •
edited
Loading

OK, so the TestLoadZeroSizeLayer test fails because the "testdata/emptyLayer.tar" archive uses the legacy format, which fails to import because there's no "manifest.json" included; updating the code to return an error for that shows;

=== FAIL: arm64.integration-cli TestDockerCLISaveLoadSuite/TestLoadZeroSizeLayer (0.04s)
    docker_cli_save_load_test.go:242: assertion failed:
        Command:  /usr/local/cli-integration/docker load -i testdata/emptyLayer.tar
        ExitCode: 1
        Error:    exit status 1
        Stdout:
        Stderr:   invalid archive: does not contain a manifest.json


        Failures:
        ExitCode was 1 expected 0
        Expected no error
    --- FAIL: TestDockerCLISaveLoadSuite/TestLoadZeroSizeLayer (0.04s)

This test was written for a situation where the user created an archive that did contain entries for (layer) files, but those files were deliberately empty;

I'm keeping the test for now as a reminder that we may want to verify "empty files in archive" cases, but we can decide to consider this an invalid situation (either files must be included and non-empty, or omitted).

thaJeztah added 3 commits July 4, 2025 11:48
@thaJeztah
image/tarexport: remove suport for loading v0/v1 images
033750c 
This removes the tarexporter.legacyLoadImage method and related helpers.
This functionality was added in 01ba0a9
(docker v1.10), which introduced the new content-addressable image
format; this code provided backward-compatibility with older archives
which contained v0/v1 images.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
image/tarexport: improve error messages for invalid archives
471f173 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
image/tarexport: inline validateManifest utility
10e9ab6 
It was just checking if a value is nil; no need to maintain a utility
for that.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah marked this pull request as ready for review July 4, 2025 10:41
@thaJeztah thaJeztah requested a review from Copilot July 4, 2025 11:20
Copilot AI reviewed Jul 4, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes legacy support for pre-Docker 1.10 (v0/v1) images and inlines simple manifest validation, updating related tests and error messages.

  • Removed tarexporter.legacyLoadImage, related helpers, and v1 compatibility code.
  • Inlined manifest nil check in load.go and updated error messages.
  • Updated integration tests to skip legacy-format archives and fixed issue links.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
integration-cli/docker_cli_save_load_test.go Updated issue URL in comment and marked legacy test as skipped.
image/v1/imagev1.go Removed deprecated v1 helpers; left only CreateID and rawJSON.
image/tarexport/tarexport.go Dropped unused legacy filename constants.
image/tarexport/load.go Inlined manifest validation, removed legacy load logic, updated error messages.
image/tarexport/load_test.go Deleted obsolete validateManifest tests.
Comments suppressed due to low confidence (1)

image/tarexport/load.go:17

  • The file now uses fmt.Errorf and errors.New but neither fmt nor errors are imported. Please add "fmt" and "errors" to the import block.
	"github.com/docker/distribution"

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 4, 2025

Whoop; all green @vvoland @cpuguy83 @tianon PTAL

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 4, 2025

[nitpick] After removing most v1 compatibility code, rawJSON is no longer referenced within this package. Consider removing this helper to reduce dead code.

Hmm... let me check; I thought it was still used somewhere

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 4, 2025

Yup; definitely still used; I have a suspicion that CoPilot ignores parts of the diff that are not shown for PRs; I've seen it do that in other cases (it considers code not shown to not exist);

moby/image/v1/imagev1.go

Lines 27 to 31 in 10e9ab6

// FIXME: note that this is slightly incompatible with RootFS logic
config["layer_id"] = rawJSON(layerID)
if parent != "" {
config["parent"] = rawJSON(parent)
}

@cpuguy83 cpuguy83 merged commit 8652cf6 into moby:master Jul 4, 2025
228 of 229 checks passed
@thaJeztah thaJeztah deleted the rm_legacy_load branch July 4, 2025 15:11
@dmcgowan dmcgowan mentioned this pull request Aug 6, 2025
Closed
@dmcgowan dmcgowan mentioned this pull request Sep 5, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@vvoland vvoland vvoland approved these changes

Assignees

No one assigned

Labels

area/images Image Distribution impact/changelog impact/deprecation status/2-code-review

Projects

None yet

Milestone

29.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@thaJeztah @cpuguy83 @vvoland