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

bake(compose): allow dot in target name #1011

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Mar 17, 2022

follow-up #1018
fixes #1009

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

@tonistiigi
Copy link
Member

I don't really like this. This is ambiguous to the --set target.args.foo and I also want to allow such selectors in HCL variable in the future. E.g. dockerfile = othertarget.dockerfile

Maybe for compose we could always replace . with _ after parsing and then also do that in bake arguments. Would that be an option?

@crazy-max
Copy link
Member Author

Maybe for compose we could always replace . with _ after parsing and then also do that in bake arguments. Would that be an option?

Sgtm as internal naming but we still display . with --print right?

@tonistiigi
Copy link
Member

Sgtm as internal naming but we still display . with --print right?

It might be better if we don't. Makes it clear that you can't use it in --set

@crazy-max
Copy link
Member Author

Sgtm as internal naming but we still display . with --print right?

It might be better if we don't. Makes it clear that you can't use it in --set

I was thinking of users that are using --print programmatically but that's an edge case.

@tonistiigi
Copy link
Member

I was thinking of users that are using --print programmatically but that's an edge case.

It shouldn't matter unless they start to parse the output and make some decisions about it. If they use it as input to other bake commands then all should work as expected.

@crazy-max crazy-max marked this pull request as draft March 23, 2022 10:20
@ciaranmcnulty
Copy link

I'd like this as currently I'm using version numbers in some target names and lack of . makes it painful.

Replacing with another char opens up a different set of ambiguities I think? What if I have targets called foo_bar and foo.bar?

It may make sense to escape dots in target names in the various places (e.g. --set 'version2\.0\.1.args.FOO=bar)

@tonistiigi
Copy link
Member

What if I have targets called foo_bar and foo.bar?

If they are considered the same then they are merged.

@crazy-max crazy-max changed the title bake: allow dot in target name bake(compose): allow dot in target name Jul 31, 2022
@crazy-max crazy-max added this to the v0.9.0 milestone Jul 31, 2022
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max crazy-max marked this pull request as ready for review August 1, 2022 09:53
@crazy-max crazy-max requested a review from jedevc August 1, 2022 11:31
Copy link
Collaborator

@jedevc jedevc left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@tonistiigi tonistiigi merged commit 30d6508 into docker:master Aug 1, 2022
@crazy-max crazy-max deleted the bake-target-name branch August 1, 2022 17:24
@ndeloof
Copy link
Contributor

ndeloof commented May 16, 2023

fix is incorrect imho. If I have a compose file to declare foo.bar and foo_bar services explicitly, bake will merge those into a single target, which is not what I've asked for. Regarding #1011 (comment) I understand you want to keep . to be an operator, but then user should be warned about conflict, not silently ignored

@tonistiigi
Copy link
Member

Detecting the conflict case sgtm

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.

bake in v.0.8.0 does not support compose services with dots
5 participants