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

introduce publish (alpha) command #10949

Merged
merged 2 commits into from Sep 7, 2023
Merged

introduce publish (alpha) command #10949

merged 2 commits into from Sep 7, 2023

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Aug 30, 2023

This introduce new docker compose (alpha) publish REPOSITORY command to publish a compose.yaml file as an OCI artifact, and support for docker:<REPOSITORY> remote resource to be used with include

illustration example:

include:
  - docker:ndeloof/test:demo
$ COMPOSE_EXPERIMENTAL_OCI_REMOTE=true docker compose config --no-normalize
name: truc
services:
  test:
    image: nginx

note: most of the changes in this PR is about passing dockerCli to the project loader

@ndeloof ndeloof force-pushed the publish branch 2 times, most recently from e9622c9 to ee6e7b9 Compare August 30, 2023 08:39
@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Patch coverage: 45.37% and project coverage change: -0.57% ⚠️

Comparison is base (aeb835a) 58.30% compared to head (057c273) 57.73%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10949      +/-   ##
==========================================
- Coverage   58.30%   57.73%   -0.57%     
==========================================
  Files         124      127       +3     
  Lines       10912    11037     +125     
==========================================
+ Hits         6362     6372      +10     
- Misses       3922     4034     +112     
- Partials      628      631       +3     
Files Changed Coverage Δ
pkg/api/api.go 28.84% <ø> (ø)
pkg/compose/publish.go 0.00% <0.00%> (ø)
pkg/remote/git.go 1.69% <0.00%> (-8.31%) ⬇️
pkg/remote/oci.go 2.46% <2.46%> (ø)
cmd/compose/events.go 28.20% <16.66%> (ø)
cmd/compose/kill.go 48.14% <25.00%> (ø)
cmd/compose/push.go 41.37% <25.00%> (ø)
cmd/compose/viz.go 51.02% <25.00%> (ø)
cmd/compose/watch.go 31.03% <25.00%> (ø)
cmd/compose/config.go 33.95% <28.57%> (-0.19%) ⬇️
... and 26 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ndeloof ndeloof force-pushed the publish branch 2 times, most recently from 1b302de to c74d06c Compare August 30, 2023 09:11
@ndeloof ndeloof requested review from a team, nicksieger, StefanScherer, ulyssessouza, glours, milas and laurazard and removed request for a team August 30, 2023 09:25
Copy link
Member

@milas milas left a comment

Choose a reason for hiding this comment

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

Logic LGTM, just a couple higher-level things

cmd/compose/compose.go Show resolved Hide resolved
}

func (g ociRemoteLoader) Accept(path string) bool {
return strings.HasPrefix(path, "docker:")
Copy link
Member

Choose a reason for hiding this comment

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

I believe elsewhere (e.g. build) we've used docker-image:// as the format

Copy link
Member

Choose a reason for hiding this comment

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

I guess this isn't technically an image, but a new artifact type, at which point I wonder if oci:// would be better?

(I know flux2 uses that format for example: https://fluxcd.io/flux/cheatsheets/oci-artifacts/#consuming-artifacts)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oci would be a better terminology from a pure implementation point of view, but I don't expect more than 1% Docker users know about Open Container Initiative, and just have a basic understanding of Docker images

Copy link
Member

Choose a reason for hiding this comment

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

Definitely agree that most people don't know what OCI is 😛

I would still say that:

  1. We should at least use a URL scheme, i.e. foo://bar instead of foo:bar, being able to do a cheap check for :// to distinguish between local & remote (without awareness of loader) is a helpful property
  2. docker isn't really descriptive and could mean many different things/is un-Googleable, but "Docker Compose OCI include" will [hopefully] get results

Choose a reason for hiding this comment

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

helm also uses oci:// for modern helm push/pull

if err != nil {
return "", err
}
return project.Name, nil
}

func (o *ProjectOptions) ToProject(services []string, po ...cli.ProjectOptionsFn) (*types.Project, error) {
func (o *ProjectOptions) ToProject(dockerCli command.Cli, services []string, po ...cli.ProjectOptionsFn) (*types.Project, error) {
if !o.Offline {
Copy link
Member

Choose a reason for hiding this comment

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

Oh this was a very elegant solution! ✨

@milas
Copy link
Member

milas commented Sep 1, 2023

Oh no, I caused a bunch of conflicts with the build changes...sorry 😭

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

Sounds good to me

@ndeloof ndeloof force-pushed the publish branch 2 times, most recently from 00dad5e to 08a7873 Compare September 6, 2023 14:05
Copy link
Member

@milas milas left a comment

Choose a reason for hiding this comment

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

LGTM, few last things but mostly the distribution/distribution/v3 dep that snuck back 😅

go.mod Outdated
@@ -50,6 +50,8 @@ require (
gotest.tools/v3 v3.5.0
)

require github.com/distribution/distribution/v3 v3.0.0-20230214150026-36d8c594d7aa
Copy link
Member

Choose a reason for hiding this comment

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

See #10954 - will need to swap things to github.com/distribution/reference to keep this from reappearing.

pkg/api/api.go Outdated
@@ -74,6 +74,8 @@ type Service interface {
Events(ctx context.Context, projectName string, options EventsOptions) error
// Port executes the equivalent to a `compose port`
Port(ctx context.Context, projectName string, service string, port uint16, options PortOptions) (string, int, error)
// Publish executes the equivalent to a `compose publish`
Publish(ctx context.Context, project *types.Project, repository string) error
Copy link
Member

Choose a reason for hiding this comment

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

Minor: we often end up refactoring the API methods to add an options object later...maybe worth having PublishOptions with a Repository string field upfront

Copy link
Member

Choose a reason for hiding this comment

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

e.g. I'm guessing this will need Quiet bool at some point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Repository being mandatory parameter should not be part of the Options struct imho

offline bool
}

const prefix = "oci:"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be oci://?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not an URL, just a prefix we decide on our own. There's no canonical URL style for OCI artifacts, as this ends being transported over http

@ndeloof ndeloof force-pushed the publish branch 2 times, most recently from 04b3fe6 to 998a7ea Compare September 6, 2023 15:24
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof ndeloof merged commit e0f39eb into docker:main Sep 7, 2023
25 checks passed
@ndeloof ndeloof deleted the publish branch September 7, 2023 05:27
@jaydrogers
Copy link

This feature seems very exciting! I found the documentation on this feature (https://docs.docker.com/engine/reference/commandline/compose_alpha_publish/) but are there more resources to learn how to use it and play around with it?

My questions:

  1. Do I need to do anything special to create a template?
  2. When I publish it, where does it go?
  3. How can I have someone else use it?
  4. Are there plans for it to work with Swarm?

@ndeloof
Copy link
Contributor Author

ndeloof commented Feb 9, 2024

docker compose -f <your compose file> alpha publish <registry>/<repo>:<tag>

then you can use this published model from another compose file using include: - oci:<registry>/<repo>:<tag>

Are there plans for it to work with Swarm?

swarm doesn't use the compose specification nor compose-go to parse compose files.
.. at least until we get docker/cli#4863 approved

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.

None yet

5 participants