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.

Sign up for GitHub

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

Jump to bottom

Add ssh to build #234

Merged
merged 8 commits into from Mar 9, 2022
Merged

Add ssh to build #234

merged 8 commits into from Mar 9, 2022

Conversation

glours
Copy link
Contributor

@glours glours commented Mar 2, 2022

What this PR does / why we need it:
Introduce ssh attribute to the build section.
This allows the usage of SSH authentification when building an image (i.e., cloning a private repo...)

Which issue(s) this PR fixes:
This PR is part of the #233 proposal

@glours glours self-assigned this Mar 2, 2022
@glours glours requested review from ndeloof, EricHripko and ulyssessouza and removed request for ndeloof, EricHripko and ulyssessouza March 2, 2022 14:50
ndeloof
ndeloof approved these changes Mar 2, 2022
View reviewed changes
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.

LGTM, with some minor notes

build.md Outdated Show resolved Hide resolved
build.md Outdated Show resolved Hide resolved
build.md Outdated Show resolved Hide resolved
ndeloof
ndeloof reviewed Mar 2, 2022
View reviewed changes
build.md Outdated Show resolved Hide resolved
@ndeloof ndeloof mentioned this pull request Mar 3, 2022
Closed
@zaucker
Copy link

zaucker commented Mar 4, 2022

Do I understand this correctly? This PR just adds the possibility to specify --ssh in the build section of the docker-compose file, but does not yet pass that to the build command upon running docker-compose build?
Just wondering if for my current project I can expect this to become functional soon or if I better don't count on it for the time being.

@glours
Copy link
Contributor Author

glours commented Mar 4, 2022

@zaucker adding the flag to the docker compose build command will be done when we'll implement this specification change in Compose

@ndeloof
Copy link
Collaborator

ndeloof commented Mar 4, 2022

implementation in docker/commose should not be a big deal, as we can rely on previous implementation by buildx bake as extension fields.

ndeloof
ndeloof reviewed Mar 4, 2022
View reviewed changes
build.md Show resolved Hide resolved
ndeloof
ndeloof reviewed Mar 4, 2022
View reviewed changes
build.md Outdated Show resolved Hide resolved
@glours
Add ssh definition to the service build section
1b8314b 
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
@glours
simplify the syntaxt section of ssh property
2b73dad 
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
@glours
change ssh type to string and update ssh section with contributors fe…
fa563cd 
…ebacks

Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
@glours
be more specific on the ssh format
b6e5db2 
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
@glours glours mentioned this pull request Mar 4, 2022
Merged
@glours
remove the empty alias to the default ssh agent in the build section
1d009e7 
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
@glours
change json spec of ssh type
52c501e 
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
ndeloof
ndeloof reviewed Mar 5, 2022
View reviewed changes
schema/compose-spec.json Outdated Show resolved Hide resolved
ndeloof
ndeloof reviewed Mar 8, 2022
View reviewed changes
build.md Outdated Show resolved Hide resolved
ndeloof
ndeloof reviewed Mar 8, 2022
View reviewed changes
build.md Outdated
build:
context: .
ssh:
default # mount the default ssh agent
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
default # mount the default ssh agent
- default # mount the default ssh agent

or

   [default]

@glours
change ssh property to a simple collection (list or dict)
8b21d37 
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
EricHripko
EricHripko approved these changes Mar 8, 2022
View reviewed changes
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.

Looks awesome ✅ Overjoyed to see SSH support coming to Compose 🙌 I've left a typo fix and some non-blocking comments inline. Feel free to land once the former is addressed.

build.md Outdated Show resolved Hide resolved
build.md Outdated Show resolved Hide resolved
build.md Outdated Show resolved Hide resolved
@glours
Copy link
Contributor Author

glours commented Mar 8, 2022

Thank you so much @EricHripko for your review, will address you comments before merging

@glours
fix typos and improvement of the ssh section readability
006c0de 
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
@ndeloof ndeloof merged commit e13d6e4 into compose-spec:master Mar 9, 2022
@glours glours mentioned this pull request Mar 10, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndeloof ndeloof ndeloof approved these changes

@levrik levrik levrik left review comments

@ulyssessouza ulyssessouza ulyssessouza approved these changes

@EricHripko EricHripko EricHripko approved these changes

Assignees

@glours glours

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@glours @zaucker @ndeloof @ulyssessouza @EricHripko @levrik