compose go v1.13.4#1753
Conversation
nicksieger
force-pushed
the
compose-go-v1.13.4
branch
from
a8a6ceb to
9c61ed3
Compare
Signed-off-by: Nick Sieger <[email protected]>
nicksieger
force-pushed
the
compose-go-v1.13.4
branch
from
9c61ed3 to
956a1be
Compare
| ConfigFiles: cfgs, | ||
| Environment: envs, | ||
| }, func(options *loader.Options) { | ||
| options.SetProjectName("bake", false) |
There was a problem hiding this comment.
There is no defaults anymore for the project name? I was thinking it was the name of the working directory?
There was a problem hiding this comment.
compose-go was raising a "project name must not be empty" error without this. Looks like the logic changed in compose-spec/compose-go#374. Might have been inadvertent, I'll check with Milas on that.
There was a problem hiding this comment.
There's a mix of "low" and "high" level loader stuff in compose-go for programmatic vs CLI usage but the separation is not clean 😔
I think we should rename and export this as something like WithDefaultProjectNameStrategy: https://github.com/compose-spec/compose-go/blob/ef6c1671ed3f3844b58f61c9f4c8e5e9cd94b5f4/cli/options.go#L447-L460
Right now that's what the ProjectFromOptions method is using, but it's not accessible if you're using loader.Load directly
There was a problem hiding this comment.
Ok, good to know. There's nothing that I'm aware of in buildx that's dependent on the latest compose-go, so does it work to file an issue and/or start a PR over there and close this in favor of a new compose-go upgrade PR later on?
There was a problem hiding this comment.
Hmm, is there any possible workaround we could do for this temporarily? I'd like to take the latest compose version in buildx v0.11 (aimed for this week or soon after), but also would be nice to make sure we take a tagged version from compose-go.
There was a problem hiding this comment.
@crazy-max @milas @nicksieger: is taking this change as-is with "bake" as the project name acceptable for the time being? If so, we can merge this and move on with the v0.11 milestone.
There was a problem hiding this comment.
I proposed the workaround by writing the code, so it's an implicit 👍 from me. I don't think the project name is ever really exposed when baking a compose file, right?
jedevc
left a comment
jedevc
left a comment
There was a problem hiding this comment.
@milas @nicksieger it would be nice to take this for v0.11 of buildx if we could, any objections?
There was a problem hiding this comment.
I think we also want to add support for the new compose features at the same time?
- Add
additional_contextstobuildservice config compose-spec/compose-go#354 - introduce
dockerfile_inlinecompose-spec/compose-go#365 - I think that's it?
They should slot neatly into compose.go, with tests in compose_test.go:
Lines 86 to 103 in 43a07f3
There was a problem hiding this comment.
Agreed, maybe they could go in a separate PR after we decide how to handle the project name default?
There was a problem hiding this comment.
Sure I'd be happy with that.
| ConfigFiles: cfgs, | ||
| Environment: envs, | ||
| }, func(options *loader.Options) { | ||
| options.SetProjectName("bake", false) |
There was a problem hiding this comment.
Hmm, is there any possible workaround we could do for this temporarily? I'd like to take the latest compose version in buildx v0.11 (aimed for this week or soon after), but also would be nice to make sure we take a tagged version from compose-go.
4 participants
No description provided.