Add support for docker load
#254
Conversation
|
The example added in the README file is the use case that I was thinking of and that we need. I'm not sure if it makes sense to use |
linus345
force-pushed
the
issue-253/docker-load
branch
from
b4b5a93 to
50bbcef
Compare
There was a problem hiding this comment.
This is a great PR! I really appreciate your splitting the huge command hook into separate files.
That said, I don't see the use-case for loading an image without actually running anything on it. The very example you show of using the load configuration also contains an image tag. And if it didn't, I'm not sure using the plugin would make sense in the first place as the docker daemon running that load step may very well not be used for anything else so making the run portion of the plugin inconditional anyways.
I would suggest that the load.sh is executed conditionally as it is now, but the run should always run no matter what.
What do you think?
|
Yes I agree with that in the current state, I'll make the run required. Although I do think it can make sense in the future if other commands like |
linus345
force-pushed
the
issue-253/docker-load
branch
2 times, most recently
from
ec6083c to
b24a21c
Compare
linus345
force-pushed
the
issue-253/docker-load
branch
5 times, most recently
from
896db83 to
bea8101
Compare
|
Thanks for the feedback. Fixed the comments now. |
.buildkite/pipeline.yml
Outdated
| - libs/** | ||
| options: | ||
| - "--color=always" | ||
| - "--external-sources" |
There was a problem hiding this comment.
After correcting the typo above, this is no longer necessary
There was a problem hiding this comment.
From the testing I did it seems that this is still needed. Shellcheck won't follow the source directives if --external-sources is not used.
There was a problem hiding this comment.
That is really weird, I ran the pipeline without the option and shellcheck did not fail. Can you try removing it so that we can further debug the issue seeing an actual run?
There was a problem hiding this comment.
Hey @linus345! thanks again for this! are you able to try toote's suggestions so we can see the issue? thanks!
There was a problem hiding this comment.
Completely missed that this comment was made.
Sure I'll try it out tomorrow.
There was a problem hiding this comment.
Sorry for taking such a long time, pushed it without this commit now.
This refactor is done to make it easier to introduce additional commands like `docker load`.
This adds support for providing a `load` argument to load a docker image before running a command.
Two new directories, `commands` and `lib`, were added and they should also be shellchecked.
linus345
force-pushed
the
issue-253/docker-load
branch
from
6f1c21f to
71464d5
Compare
|
Finally got around to fixing the comments. |
Sourcing the shared lib script allows shellcheck to see variables that are defined which is needed to prevent shellcheck from complaining about referencing unassigned variables. [SC2154](https://www.shellcheck.net/wiki/SC2154). `result is referenced but not assigned`
linus345
force-pushed
the
issue-253/docker-load
branch
from
71464d5 to
4a4d2fb
Compare
3 participants
This PR is an attempt to implement support for
docker load.Related issue #253.
It first makes a small refactor by moving out the
runcommand code into its own file and making thehooks/commandfile only the entry point. This is the same structure that buildkite-plugins/docker-compose-buildkite-plugin uses. It then introduces a new file for thedocker loadcommand.