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

Container Build: Add tar.gz (Docker Archive) file writing #35151

Merged
merged 5 commits into from Sep 7, 2023

Conversation

Danielku15
Copy link
Contributor

@Danielku15 Danielku15 commented Sep 5, 2023

Fixes dotnet/sdk-container-builds#283, dotnet/sdk-container-builds#431 (partially)

This PR introduces a new feature which allows developers to write tar.gz archives as understood by Docker (and podman) in a docker load operation.

The effective changes are:

  • Extended the DestinationImageReference to hold the ILocalRegistry like the Registry (representing the remote registry) and also providing a way to determine which kind of refernce we are having (similar to the OneOf generation in Protobuf).
  • Added a ILocalRegistry implementation which dumps the tar.gz to disk (creating of these archives already exists from the DockerCli implementation)
  • Add a new MSBuild Parameter ArchiveOutputPath which developers would point to the absolute path where they wanna have the archive written.
  • Added an integration test for this workflow.
  • Reorganized a bit the code regarding the different DestinationImageReferenceKinds

To be discussed and agreed:

  • The final naming of the property
  • Considering adapting the logs and error messages.
  • Mid term (not this PR): Support of OCI Image archives beside the Docker Archive
  • Long term (not this PR): would be great if we can internally unify the ContainerBuilder and CreateNewImage workflows. Didn't like this duplication of code and workflow - DRY is a bit violated here and it also makes testing tricky.

@Danielku15 Danielku15 requested a review from a team as a code owner September 5, 2023 15:39
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Containers Related to dotnet SDK containers functionality untriaged Request triage from a team member labels Sep 5, 2023
@baronfel
Copy link
Member

baronfel commented Sep 5, 2023

I added a test leg to baronfel/sdk-container-demo for this, and you can see the results of the new job here. It looks like the archive file creation logic is clobbering or doesn't do correct permissions checks or something though? Here's the error I saw: https://github.com/baronfel/sdk-container-demo/actions/runs/6087233076/job/16515358596#step:4:31

@Danielku15
Copy link
Contributor Author

Danielku15 commented Sep 6, 2023

I tweaked a bit the output path handling. I think the problem in your case was that you tried to create use a non-existing directory path in your test. Generally I rather thought that people should/will use a path down to a file so they know also the file name afterwards.

The folder name was rather a fallback for use case where we wanna set a common property from global level (e.g. via msbuild property). I think this lead to an issue that we tried to write to a file which has a directory path.

I added some further checks just for safety purposes.

Here a sneak peek of the next upcoming PR on top of this. But I want to handle each topic and feature individually.

@baronfel
Copy link
Member

baronfel commented Sep 6, 2023

How should we treat repository names that contain slashes? Right now they create subdirectories, and I'm not sure if that's expected or not.

@baronfel
Copy link
Member

baronfel commented Sep 6, 2023

@baronfel
Copy link
Member

baronfel commented Sep 6, 2023

@Danielku15 this looks good to me, with two hopefully minor requests:

  • can you add a log line for the created tar.gz file, either as a trace or by updating the to say something different when an archive is created instead of pushing to the local daemon? It would make it easier/clearer to see where the artifact is. Maybe a message format like Pushed image '<image name and tag>' to a local archive at '<location>' would be clear for users?
  • can you add the full path to the newly-created file as an output of the target, and expose it in the targets? That way people doing MSBuild automation (or using the MSBuild CLI property/item evaluation that will ship in RC2) can easily get access to that item.

@Danielku15
Copy link
Contributor Author

Danielku15 commented Sep 6, 2023

@baronfel

can you add a log line for the created tar.gz file, either as a trace or by updating the to say something different when an archive is created instead of pushing to the local daemon? It would make it easier/clearer to see where the artifact is. Maybe a message format like Pushed image '' to a local archive at '' would be clear for users?

I will update the text. Currently it is indeed a bit cryptic. Maybe I can utilize the information from the ILocalRegistry and DestinationImageReference to build a meaning full text using placeholders.

Are there any policies or conventions to follow on those messages?

can you add the full path to the newly-created file as an output of the target, and expose it in the targets? That way people doing MSBuild automation (or using the MSBuild CLI property/item evaluation that will ship in RC2) can easily get access to that item.

The new property is an external input to the SDK. I would expect the path is already available via $(ContainerArchiveOutputPath) everywhere. This is similar to the ContainerRegistry or ContainerImageTags property. Or am I missing something?

@baronfel
Copy link
Member

baronfel commented Sep 6, 2023

Are there any policies or conventions to follow on those messages?

You should create a format string with placeholders for the two variables in the Strings.resx file and the build (build.cmd/sh) should create matching translation entries for the other languages automatically on your behalf. That should be the biggest portion :)

new file path output

My thinking here was my use case - where I specify a folder. In that case the outputs wouldn't be directly known and would require that the user reconstruct them from the other inputs (path + repository name, etc) and it's just nicer to emit outputs from the target instead in these cases. This is the same reason we emitted the digest as an output, etc.

@Danielku15
Copy link
Contributor Author

Danielku15 commented Sep 6, 2023

@baronfel Updated the code in 0a820fe. Unfortunately I have to rely on runtime type checks to extract the output path from the ILocalRegistry implementation into the task. Not really nice OOP practice but having a decoupled way of filling output parameters on the task from within the workflow is likely too much for this PR.

Copy link
Member

@baronfel baronfel left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Containers Related to dotnet SDK containers functionality untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants