-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
4e90f74
to
6789c6f
Compare
fc13b27
to
9e994a1
Compare
9e994a1
to
be830ff
Compare
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.
Just some suggestions and questions for debate.
Also, since it seems that the pipeline is not doing this for some reason (I thought I had configured it to do so 🤔), please run shellcheck on the scripts just to make sure everything is fine and paste the results here. Thanks.
(Nevermind the comment about shellcheck, it seems that I confused myself with the Makefile lol) |
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.
Does not work for multiple docker arguments. I had no issue with this PR only tackling volumes and the next PR making the config more modular to allow for arbitrary docker flags.
However, as it stands, this cannot be merged since it will cause further configuration to break.
We should either fix this to work for multiple arguments or rollback to just configuring volumes.
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.
Needs to be tested in the server.