Skip to content

Conversation

@tonistiigi
Copy link
Member

part of #2711

cmd := &cobra.Command{
Use: "import [OPTIONS] < bundle.dockerbuild",
Short: "Import a build into Docker Desktop",
Args: cobra.NoArgs,
Copy link
Member

@crazy-max crazy-max Mar 5, 2025

Choose a reason for hiding this comment

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

I think we should use args as file input instead of a flag and require at least one arg. Multiple files should be supported like Docker Desktop does atm imo:

docker buildx history import FILE...

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not usually the convention I think when the file is optional and defaults to stdin. If file is arg then for stdin, the user should write import -

Copy link
Member Author

Choose a reason for hiding this comment

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

Importing multiple files together, I'm ok with.

Copy link
Member

Choose a reason for hiding this comment

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

Ah indeed I missed it was reading from stdin. Sounds good to support multiple --file then

Comment on lines 84 to 86
if i == 0 {
err = browser.OpenURL(url)
}
Copy link
Member

Choose a reason for hiding this comment

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

If multiple files are provided I think we should skip this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

Copy link
Member

Choose a reason for hiding this comment

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

Because who knows what user wants to see? Is it the last one or the first one? But I guess that's fine to open the very first imported record in Docker Desktop.

Copy link
Member

Choose a reason for hiding this comment

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

Although I wonder if there are more than one record imported we could instead open the Builds view and filter by imported builds 🤔

Comment on lines 17 to 23
if os.Getenv("WSL_DISTRO_NAME") != "" {
return "unix://" + filepath.Join(wslSocketPath, socketName), nil
}
Copy link
Member

@crazy-max crazy-max Mar 7, 2025

Choose a reason for hiding this comment

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

Pushed this change for testing WSL support with dev build of Docker Desktop but doesn't work yet. I'm taking a look if this can be solved for next Docker Desktop release but in the meantime we could disable support for import command on WSL and explain that it should be invoked on Windows host instead while this is being investigated.

Copy link
Member

Choose a reason for hiding this comment

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

Updated to check if socket path exists and returns error if not:

$ docker buildx history import < rec-20250119-docker~build-push-action~JBMEH5.dockerbuild.zip
ERROR: Docker Desktop Build backend is not supported on WSL. Please run this command on Windows host instead.

@tonistiigi tonistiigi merged commit 23afb70 into docker:master Mar 10, 2025
140 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants