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

Add support for arbitrary CLI build commands to be passed through to the CLI builder. #145

Closed
wants to merge 2 commits into from

Conversation

zoonage
Copy link

@zoonage zoonage commented Feb 24, 2021

What this PR does / why we need it: Add support for arbitrary CLI build commands to be passed through to the CLI builder.

Which issue(s) this PR fixes: docker/compose#7997

Fixes docker/compose#7997

@EricHripko
Copy link
Collaborator

Hi @zoonage - thank you for putting this contribution together 👍 It looks like you're aiming to enable SSH secrets, which is being discussed under #81. Do you think that your request will be satisfied by the linked issue?

@zoonage
Copy link
Author

zoonage commented Mar 26, 2021

Hi @zoonage - thank you for putting this contribution together 👍 It looks like you're aiming to enable SSH secrets, which is being discussed under #81. Do you think that your request will be satisfied by the linked issue?

It's turned into more of a "allow arbitrary CLI arguments to be passed through" than something specifically focussed on mounting the SSH agent into the container now

Signed-off-by: Aaron George <aaron@ometria.com>
@EricHripko
Copy link
Collaborator

cc @kohidave too

Copy link
Collaborator

@EricHripko EricHripko left a comment

Choose a reason for hiding this comment

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

Given that we're trying to keep the Compose spec somewhat agnostic of the implementation, I'm concerned about the portability of this addition as it'd essentially lock the Compose file into a single CLI implementation. Would welcome hear thoughts from other maintainers on this though 👀

@zoonage
Copy link
Author

zoonage commented Mar 26, 2021

Given that we're trying to keep the Compose spec somewhat agnostic of the implementation, I'm concerned about the portability of this addition as it'd essentially lock the Compose file into a single CLI implementation. Would welcome hear thoughts from other maintainers on this though 👀

My thinking behing this is that it's hard to strike a balance between being agnostic and having support for new features in Docker as soon as they come out (especially after BuildKit released quite a few things at once).

That said I can change this to specify ssh, secret mounts, and anything else based on what is decided, either way is going to be pretty much the same amount of effort

@ndeloof
Copy link
Collaborator

ndeloof commented Mar 30, 2021

My :-1 on this. Passing arbitrary command obviously would locally unlock features but would make it impossible to adapt to another build context.

Let's imaging we want to provide support for Google Cloud Build. If the build require ssh keys, then it's highly probable they don't just expose buildx command line flags, but offer another way to setup credentials. With adequate abstraction, we can adapt to the infrastructure.

As a Jenkins contributor I had to fight many times with the lack of abstraction around withDocker which basically expose the CLI flags, and make it impossible to adopt an alternate approach.

I'm convince we should not hurry here, and just adopt the buildkit features with adequate declarative abstraction. In the meantime, just a reminder we support x- extensions if you want to experiment a syntax to introduce ssh support in docker-compose.

Copy link
Collaborator

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

:-1 as explained on #145 (comment)

would better first open an issue on the spec to debate this
also need to update the specification docs if a new field is introduced

@zoonage
Copy link
Author

zoonage commented Mar 31, 2021

:-1 as explained on #145 (comment)

would better first open an issue on the spec to debate this
also need to update the specification docs if a new field is introduced

Created #157 for discussion, should I leave this PR open for updating after the discussion or close it and open a new one later?

@ndeloof
Copy link
Collaborator

ndeloof commented Mar 31, 2021

can be kept open, let's wait for decisions on #157

@ndeloof ndeloof removed the request for review from justincormack April 23, 2021 12:40
@zoonage
Copy link
Author

zoonage commented Jul 5, 2021

Closing this out as I don't have capacity to make any progress on this and haven't for a while

@zoonage zoonage closed this Jul 5, 2021
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

3 participants