-
Notifications
You must be signed in to change notification settings - Fork 103
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 ResourceResolver to accept remote resources #435
Conversation
e63912e
to
6d059a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, just left a couple nits/a question
ec2ed71
to
61a94cb
Compare
added example code using this feature to support git resources: docker/compose#10811 |
// 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 string
s this won't be very relevant afaict (not to mention the required refactoring is far from being trivial)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this 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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
loader/include.go
Outdated
@@ -65,7 +74,7 @@ func loadInclude(configDetails types.ConfigDetails, model *types.Config, options | |||
return nil, err | |||
} | |||
|
|||
imported, err := load(types.ConfigDetails{ | |||
imported, err := load(nil, types.ConfigDetails{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imported, err := load(nil, types.ConfigDetails{ | |
imported, err := load(ctx, types.ConfigDetails{ |
loader/loader.go
Outdated
@@ -193,8 +204,13 @@ func parseYAML(source []byte) (map[string]interface{}, PostProcessor, error) { | |||
return converted.(map[string]interface{}), &processor, nil | |||
} | |||
|
|||
// Load reads a ConfigDetails and returns a fully loaded configuration | |||
// Load deprecated, prefer LoadWithContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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
3a240cc
to
88eace5
Compare
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
88eace5
to
ad9be94
Compare
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ResourceLoaders
can be configured by Compose implementation to support remote resources withextends
andinclude
. This will allow users to rely on compose.yaml files published by others, without the need for a mono-repo.illustration example:
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
andsecret.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