Skip to content

Docker implementation#1219

Draft
Jrice1317 wants to merge 48 commits into
conda:mainfrom
Jrice1317:docker-implementation
Draft

Docker implementation#1219
Jrice1317 wants to merge 48 commits into
conda:mainfrom
Jrice1317:docker-implementation

Conversation

@Jrice1317
Copy link
Copy Markdown
Contributor

@Jrice1317 Jrice1317 commented Apr 22, 2026

Description

This PR adds Docker output support to constructor, generating a ready-to-use Dockerfile and optionally building the resulting image via installer_type: docker and docker_build: true in the construct.yaml.

Changes

New:

  • constructor/docker_build.py: Handles Docker output by rendering template and optionally building portable image
  • constructor/dockerfile_template.tmpl: Template used to generate Dockerfile
  • examples/docker_build/construct.yaml: Example construct.yaml used for testing

Updated:

  • constructor/_schema.py: Adds docker to installer_type and adds docker_base_image, docker_tag, docker_labels
  • constructor/main.py: Adds docker to installer types
  • tests/test_examples.py: Adds test_docker_build to cover Dockerfile generation, image build and smoke test

Notes:

  • When docker_build is True, the user can start using the resulting portable image by running the command docker load -i <image_name>
  • Verified RuntimeError is raised when base_image is not provided
RuntimeError: Base image for Dockerfile not specified. Please set 'docker_base_image' in construct.yaml, e.g.:
 docker_base_image: debian:13.4-slim@sha256:4ffb3a1511099754cddc70eb1b12e50ffdb67619aa0ab6c13fcd800a78ef7c7a

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@Jrice1317 Jrice1317 requested a review from a team as a code owner April 22, 2026 19:32
@github-project-automation github-project-automation Bot moved this to 🆕 New in 🔎 Review Apr 22, 2026
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Apr 22, 2026
@Jrice1317 Jrice1317 marked this pull request as draft April 22, 2026 19:45
@Jrice1317 Jrice1317 changed the title Docker implementation Docker implementation [skip windows] May 6, 2026
@Jrice1317 Jrice1317 force-pushed the docker-implementation branch from 9fa7db8 to 9f20cbe Compare May 6, 2026 16:10
@Jrice1317 Jrice1317 changed the title Docker implementation [skip windows] Docker implementation May 8, 2026
@Jrice1317 Jrice1317 marked this pull request as ready for review May 8, 2026 23:01
Comment thread constructor/_schema.py Outdated
"""
Base image to use for docker builds when `installer_type` includes `docker` or `docker_build` is True.
Should be a specific image reference. For reproducibility, please specify a SHA256 digest.
For example: `debian:13.4-slim@sha256:abc123...`. If the digest is not provided, a warning will be shown.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The example is great, but reproducibility is not the primary concern here either, it's supply chain security. I would overall do don't think it's constructor's responsibility to decide or advise on the user's security model though, even if it's just a warning. Warnings show up in GitHub Action summaries and can introduce unwanted noise.

Copy link
Copy Markdown
Contributor Author

@Jrice1317 Jrice1317 May 13, 2026

Choose a reason for hiding this comment

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

So, remove the SHA256 warning altogether? Or just rephrase it to be neutral?

Base image to use for docker builds when `installer_type` includes `docker` or `docker_build` is True.
    Should be a specific image reference. For example: `debian:13.4-slim@sha256:abc123...`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would remove this altogether, so remove "Should be...", but definitely keep the example as is.

Comment thread constructor/_schema.py Outdated
"""
If `True`, builds a docker image using the Dockerfile generated by constructor and saves it as a portable tarball.
``<name>-<version>-<platform>.tar`` will be created in the output docker directory.
Requires `docker_base_image` to be specified.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The base image should be a mandatory parameter in any case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I'm following here as the base image being required is already enforced. Is saying Requires `docker_base_image` to be specified. misleading?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would call it redundant since all Docker parameters require the base image to be set.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ahh, it is true it's required, but I think it's helpful for those who don't know the code and just look at the schema.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the right place to declare that is with docker_base_image, not docker_build (or why in docker_build and not the other docker options?). You could write in docker_base_image that this is a required parameter to make it very explicit.

Comment thread constructor/_schema.py Outdated
The labels `org.opencontainers.image.title` and `org.opencontainers.image.version` are
set automatically from `name` and `version`.
"""
docker_build: bool = False
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given that Docker provides several output formats, I would rather choose something that is extendable. It's okay if we don't support all formats right now, we can disclose that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can you please clarify what you have in mind here? Are you suggesting we do something like docker_output: tarball in place of docker_build: True?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correct, that's what I suggest. The available options should correspond verbatim to the same option Docker supports (uncompressed, etc.).

Comment thread constructor/_schema.py
message: "This base environment is frozen and cannot be modified."
```
"""
docker_base_image: Annotated[str, Field(min_length=1)] | None = None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of prefixing all fields with docker_*, would it make more sense to make docker its own key with all other properties being a subkey? I know that's not our current convention, so I'm not 100% sure about that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree it could be cleaner, but unless we can do a broader refactor, I think docker_* keeps things consistent with how it's handled today.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also, please volunteer me if/when we can do a schema refactor. (:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sticking with the current schema is a fair choice. A schema refactor would constitute a major breaking change that I don't foresee ourselves doing unless we absolutely have to.

Comment thread constructor/_schema.py Outdated
docker_tag: NonEmptyStr | None = None
"""
Tag to use for the docker image.
If not provided, it will default to `<name>:<version>`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need to provide a default here? docker build runs with a tag, too.

Will this require docker_build (or whatever successor you choose) to be set?

Copy link
Copy Markdown
Contributor Author

@Jrice1317 Jrice1317 May 14, 2026

Choose a reason for hiding this comment

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

I don't think we should. It's handled in the script, but I could add something like Has no effect if `docker_build` or `docker_output` is not set.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I actually wrote the opposite of what I meant to write: docker build runs without a tag, too. So, tagging the image isn't necessarily required for it to work. Definitely disclose in the schema when that option has no effect for the docker installer type.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

To clarify, it is highly suggested that a tag be used with docker build. The only thing is that it is not required for the user to come up with that tag because it will default to miniconda3:25.1.1 for example. Please let me know if that default is sufficient or if something like miniconda3-25.1.1:linux-arm64 would be better.

Copy link
Copy Markdown
Contributor Author

@Jrice1317 Jrice1317 May 14, 2026

Choose a reason for hiding this comment

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

Not sure if there's a limit on characters for the tag name, but the latter is definitely more descriptive. 😄

Comment thread constructor/dockerfile_template.tmpl Outdated

LABEL org.opencontainers.image.title="{{ name }}"
LABEL org.opencontainers.image.version="{{ version }}"
{%- if labels %}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think the if is needed. If labels is empty, the loop just won't run.

Comment thread constructor/main.py Outdated
Comment on lines +56 to +65
if docker_build and osname == "win":
sys.exit(
"Error: 'docker_build' is not supported on Windows. "
"Run the build on Linux or macOS instead."
)
if docker_build and itype in ("pkg", "exe"):
sys.exit(
"Error: 'docker_build' is not compatible with installer_type 'pkg' or 'exe'. "
"Use installer_type: 'sh', 'docker', or omit installer_type."
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of erroring out, we should just ignore those.

Comment thread constructor/main.py
Comment on lines +438 to +440
info["_outpath"] = abspath(join(output_dir, get_output_filename(info))).replace(
".docker", ".sh"
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aren't you already doing this in the build function?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right. I'll remove it from docker_build.py and keep the path handling here so it's consistent with how the other installer types manage _outpath.

Comment thread tests/test_examples.py Outdated

@pytest.mark.skipif(sys.platform.startswith("win"), reason="Unix only")
@pytest.mark.skipif(not shutil.which("docker"), reason="Docker not available")
def test_docker_build(tmp_path):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This test doesn't test the labels.

Comment thread constructor/main.py
),
"win": ("exe",),
}
all_allowed = set(sum(os_allowed.values(), ("all", "docker")))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We only allow Docker for Linux platforms and can only build them on Linux and macOS.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Does this mean I could do

os_allowed = {
        "linux": ("sh", "docker"),
        "osx": (
            "sh",
            "pkg",
        ),
        "win": ("exe",),
    }

and it would cover all the bases since constructor handles the cross-platform checks for us?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I had this originally, but wasn't sure how to test the cross-platform check properly

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That would add docker to the all list for Linux, which may not be what you want. The platform here is actually the CLI argument, so cross-platform checks are done automatically. You may just need to write:

if itype == "docker" and osname == "linux":
    return (itype,)
elif ...

@lrandersson
Copy link
Copy Markdown
Contributor

@Jrice1317 I'd be happy to do a review once the items from Marco have been resolved, it'll also help me understand the changes better

Co-authored-by: Marco Esters <mesters@anaconda.com>
@Jrice1317 Jrice1317 requested a review from marcoesters May 14, 2026 15:44
@Jrice1317 Jrice1317 marked this pull request as draft May 14, 2026 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed [bot] added once the contributor has signed the CLA

Projects

Status: 🆕 New

Development

Successfully merging this pull request may close these issues.

4 participants