Skip to content
This repository has been archived by the owner on May 23, 2024. It is now read-only.

Add support for docker arguments #27

Merged
merged 7 commits into from
Mar 28, 2022
Merged

Add support for docker arguments #27

merged 7 commits into from
Mar 28, 2022

Conversation

bdmendes
Copy link
Contributor

@bdmendes bdmendes commented Feb 9, 2022

Needs to be tested in the server.

@bdmendes bdmendes force-pushed the support-docker-volumes branch from 4e90f74 to 6789c6f Compare February 9, 2022 09:28
@bdmendes bdmendes force-pushed the support-docker-volumes branch 5 times, most recently from fc13b27 to 9e994a1 Compare February 9, 2022 11:36
@bdmendes bdmendes force-pushed the support-docker-volumes branch from 9e994a1 to be830ff Compare February 9, 2022 11:37
deployments/project-configs.sh Outdated Show resolved Hide resolved
deployments/project-configs.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@miguelpduarte miguelpduarte left a 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.

deployments/deploy-types.sh Outdated Show resolved Hide resolved
deployments/deploy-types.sh Outdated Show resolved Hide resolved
deployments/project-configs.sh Outdated Show resolved Hide resolved
@miguelpduarte
Copy link
Collaborator

(Nevermind the comment about shellcheck, it seems that I confused myself with the Makefile lol)

deployments/project-configs.sh Outdated Show resolved Hide resolved
@DoStini DoStini self-requested a review March 14, 2022 17:40
@imnotteixeira imnotteixeira removed their request for review March 15, 2022 19:10
Copy link
Collaborator

@miguelpduarte miguelpduarte left a 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.

deployments/deploy-types.sh Outdated Show resolved Hide resolved
deployments/deploy.sh Outdated Show resolved Hide resolved
deployments/deploy.sh Outdated Show resolved Hide resolved
deployments/project-configs.sh Outdated Show resolved Hide resolved
deployments/project-configs.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@miguelpduarte miguelpduarte left a comment

Choose a reason for hiding this comment

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

xargs seems like it would work here:

image

We just need to be mindful of spaces, potentially quoting the passed in flags, as necessary -- for example, in case the volumes path has spaces in the filepath:

image

@bdmendes bdmendes merged commit 4801cf6 into master Mar 28, 2022
@miguelpduarte miguelpduarte changed the title Add support for docker volumes Add support for docker arguments Mar 28, 2022
@bdmendes bdmendes deleted the support-docker-volumes branch August 15, 2022 14:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants