-
Notifications
You must be signed in to change notification settings - Fork 483
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
feat: add support for ssh property in the build command #1058
feat: add support for ssh property in the build command #1058
Conversation
8b3cf3d
to
de03e45
Compare
"test_build_ssh_map", | ||
"test_build_ssh_array", | ||
]) | ||
|
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 think it would make sense to produce some output that shows that information was really acquired from the agent and then assert it in the test.
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.
do you mean not having the mock agent at all?
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.
sorry, got it, I found a solution to check the key
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.
Sorry for not being explicit, but your solution was pretty similar to what I was thinking about.
A more simplier solution would be to do:
RUN echo "No id:" >> /result.log
RUN --mount=type=ssh ssh-add -L >> /result.log
RUN echo "id1:" >> /result.log
RUN --mount=type=ssh,id=id1 ssh-add -L >> /result.log
RUN echo "id2:" >> /result.log
RUN --mount=type=ssh,id=id2 ssh-add -L >> /result.log
Then in docker compose set command to cat /result.log
and then check the logs in the test via podman compose logs
(there are a few tests that do this to use for reference).
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.
Done
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.
This PR is great, thank you! I had a couple of suggestions for the test, but overall looks great.
5327f5c
to
008d8ef
Compare
008d8ef
to
a2d0908
Compare
Could you please combine the commits into one and remove |
a2d0908
to
f455114
Compare
Fixes containers#705: Add support for ssh property in the build command Signed-off-by: Domenico Salvatore <[email protected]>
f455114
to
ab33954
Compare
Fixes #705: Add support for ssh property in the build command
Schema definition: https://github.com/compose-spec/compose-spec/blob/6f87f9328842307d33dc777ca243cf92acf7aea4/schema/compose-spec.json#L107