introduce ResourceResolver to accept remote resources by ndeloof · Pull Request #435 · compose-spec/compose-go

ndeloof · 2023-07-13T10:06:11Z

ResourceLoaders can be configured by Compose implementation to support remote resources with extends and include. This will allow users to rely on compose.yaml files published by others, without the need for a mono-repo.

illustration example:

include:
   - git@github.com:acme/commons.git#v2.0:compose.yaml

If this feature demonstrates some significant use for a specific protocol (git, http, oci-artifacts ?) we could make those part of the compose specification and offer a default loader implementation in compose-go

We also could extend this support to other places compose loads files, typically with env_file and secret.file but I'd prefer we keep this focussed on a simpler use-case for a first round.

https://docker.atlassian.net/browse/ENV-251

loader/loader.go

laurazard

LGTM overall, just left a couple nits/a question

cli/options.go

loader/testdata/remote/compose.yaml

loader/loader.go

ndeloof · 2023-07-17T13:06:12Z

added example code using this feature to support git resources: docker/compose#10811

loader/paths.go

milas · 2023-07-17T16:28:35Z

loader/loader.go

+	// Accept returns `true` is the resource reference matches ResourceLoader supported protocol(s)
+	Accept(path string) bool
+	// Load returns the path to a local copy of remote resource identified by `path`.
+	Load(ctx context.Context, path string) (string, error)


Would it make sense to actually return the contents as []byte here? Internally, loader implementations could do on-disk caching for performance, but externally it could be more of a "store"/VFS interface.

That'd let us use this consistently in all code-paths, aka have a FilesystemLoader as the default/fallback. (Not suggesting that as part of this PR, just looking forward.)

That'd make testing easier in some cases, and hopefully help address some of the implicit direct I/O that happens, e.g. include / extends / file references in secrets, env_file, etc

I had this in mind first, but there are many places we also load sibling content, and as such we really need a path so we can search within parent folder.

Right, I had the same thought and then ended up looking around and deciding that returning a local path might make more sense in the long run. Okay with both implementations though.

Joining @milas on this, I would actually love to see some Afero in action here.
It looks like the CopyOnWriteFs would look great for this use case.
And it would handle the case of sibling files too.

Ah yeah the sibling content makes sense and needs more thought. I guess a good first step (independent of this) would be to virtualize/abstract the direct filesystem operations happening and could then offer an updated loader interface that works with that.

@ulyssessouza Good reminder about Afero, we could definitely make interfaces compatible with them in compose-go so it's easy to plug their implementations in. There's also the newer io.FS stuff in Go which might work since compose-go only reads but never writes.

I agree it would be nice we could rely on some higher level abstraction like the io.fs package, but as this all results into a types.ConfigDetails which uses file paths as strings this won't be very relevant afaict (not to mention the required refactoring is far from being trivial)

@ulyssessouza @milas we can't use such an abstraction as we need an actual file path to populate attributes we pass directly to engine API, like build context or bind mount source.

glours

Sounds good to me 👍

milas

As a POC, it LGTM, but this does go against the spec: https://docs.docker.com/compose/compose-file/05-services/#finding-referenced-service

file value can be:

Not present. This indicates that another service within the same Compose file is being referenced.

File path, which can be either:

Relative path. This path is considered as relative to the location of the main Compose file.

Absolute path.

Everything in the spec is about file paths currently and currently compose-go is all that's needed to load any arbitrary Compose file.

With this change, if a Compose file gets loaded that uses remote resources, if the caller doesn't have an appropriate loader in place, it will silently fallback to trying to make things file paths, e.g. https://example.com/compose.yaml -> /Users/milas/myproject/https://example.com/compose.yaml and then fail.

At a minimum, I think we need to define an unambiguous distinction in the spec between local file paths and remote resources, e.g. remote resource must start with a protocol ([a-z]+://), and if no loader is registered for the desired protocol, an error is returned. Values without a protocol (or an explicit file://) could continue to be treated as file paths.

extends.file could be the first part then updated to reference "a resource path" as a generic concept, for example.

Beyond that, I'd also argue we should consider having a standard set of remote loaders (http(s), git) live in SUBmodules of compose-go so that they're usable outside of docker/compose.

milas · 2023-07-21T13:18:07Z

loader/include.go

 		}

-		imported, err := load(types.ConfigDetails{
+		imported, err := load(nil, types.ConfigDetails{


Suggested change

imported, err := load(nil, types.ConfigDetails{

imported, err := load(ctx, types.ConfigDetails{

milas · 2023-07-21T13:19:46Z

loader/loader.go

 }

-// Load reads a ConfigDetails and returns a fully loaded configuration
+// Load deprecated, prefer LoadWithContext


Suggested change

// Load deprecated, prefer LoadWithContext

// Load reads a ConfigDetails and returns a fully loaded configuration.

//

// Deprecated: use LoadWithContext.

This should make it show up as deprecated in IDEs and hide it on pkg.dev

loader/testdata/remote/compose.yaml

loader/loader_test.go

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

ndeloof requested review from glours, laurazard, milas and ulyssessouza July 13, 2023 10:12

ulyssessouza reviewed Jul 13, 2023

View reviewed changes

loader/loader.go Outdated Show resolved Hide resolved

ndeloof force-pushed the remote_resource branch 6 times, most recently from e63912e to 6d059a4 Compare July 17, 2023 08:40

laurazard approved these changes Jul 17, 2023

View reviewed changes

cli/options.go Outdated Show resolved Hide resolved

loader/testdata/remote/compose.yaml Outdated Show resolved Hide resolved

loader/loader.go Outdated Show resolved Hide resolved

ndeloof force-pushed the remote_resource branch 2 times, most recently from ec2ed71 to 61a94cb Compare July 17, 2023 12:48

ndeloof mentioned this pull request Jul 17, 2023

add support for git remote resources docker/compose#10811

Merged

milas reviewed Jul 17, 2023

View reviewed changes

ndeloof force-pushed the remote_resource branch from 61a94cb to 61589c4 Compare July 18, 2023 14:14

glours approved these changes Jul 21, 2023

View reviewed changes

milas reviewed Jul 21, 2023

View reviewed changes

ndeloof force-pushed the remote_resource branch 2 times, most recently from 3a240cc to 88eace5 Compare August 7, 2023 06:29

introduce ResourceResolver to accept remote resources

ad9be94

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

ndeloof force-pushed the remote_resource branch from 88eace5 to ad9be94 Compare August 7, 2023 06:29

ndeloof requested a review from milas August 8, 2023 14:02

ndeloof merged commit 339d49d into compose-spec:master Aug 9, 2023

ndeloof added a commit to ndeloof/compose-go that referenced this pull request Aug 10, 2023

Merge pull request compose-spec#435 from ndeloof/remote_resource

8e131a4

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

	imported, err := load(nil, types.ConfigDetails{
	imported, err := load(ctx, types.ConfigDetails{

-// Load deprecated, prefer LoadWithContext
+// Load reads a ConfigDetails and returns a fully loaded configuration.
+//
+// Deprecated: use LoadWithContext.

Conversation

ndeloof commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

laurazard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ndeloof commented Jul 17, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glours left a comment

Choose a reason for hiding this comment

Uh oh!

milas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ndeloof commented Jul 13, 2023 •

edited

Loading