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.

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

Make it easier to pass additional volume bindings into run.sh #675

Closed
matentzn opened this issue Aug 29, 2022 · 9 comments · Fixed by #678
Closed

Make it easier to pass additional volume bindings into run.sh #675

matentzn opened this issue Aug 29, 2022 · 9 comments · Fixed by #678
Assignees

Comments

@matentzn
Copy link
Contributor

This way, we do not need to hack run.sh when we need to, for example, bind config files to tools which are not located in the working directory.

@gouttegd
Copy link
Contributor

Something like

ODK_BINDS=<host-directory>:<mounting-point>,<host-directory>:<mounting-point> sh run.sh make ...

?

@matentzn
Copy link
Contributor Author

since I think most volume binding efforts are more permanent, I was thinking of adding this to the config file:

docker_config:
    volume_bindings:
        - /.../dir1:/.../dir1
        - /.../dir2:/.../dir2

And the manifest the bindings with Jinja. That way the run.sh does not get too many variable to keep track off..

@gouttegd
Copy link
Contributor

This would make the behaviour dependent on absolute directories from one editor’s machine (the editor who manages the config file and runs the update_repo script)… Do we really want that?

@matentzn
Copy link
Contributor Author

Very good point.. No. I guess not. But you could add recommendation for PWD syntax?

docker_config:
    volume_bindings:
        - $PWD/../../dir1:/.../dir1

@gouttegd
Copy link
Contributor

gouttegd commented Aug 31, 2022

If it’s expected that the most common use case will be to bind directories located somewhere in the repository, we could make that the default?

That is, allow things like

docker_config:
  volume_bindings:
    - dir1:/.../dir1

and treat dir1 as relative to the top-level directory of the repository, by automatically prepending $PWD/../../.

Conversely, if the specified directory is absolute ( - /somewhere/dir1:/.../dir1), then use it as it is and assume the editor knows what they are doing.

@matentzn
Copy link
Contributor Author

The more you ask about the less clear it is to me how advantages it is to make it configurable. My current use case is a binding outside the repo, but its so edge case.. Also cross-platform wont work with the config file approach.

Maybe a generic ODK_DOCKER_OPTIONS="" which can be used for arbitrary docker parameters us actually the best.. and easiest?

@gouttegd
Copy link
Contributor

gouttegd commented Aug 31, 2022

The (slight) advantage of a variable specifically intended for bindings (ODK_BINDS instead of a more generic ODK_DOCKER_OPTIONS) is that it would dispense users from having to know the docker run option to specify a binding (-v) — they would just specify the directories to bind, then the run.sh script would use the correct option.

ODK_BINDS could also be made to work transparently with singularity as well as docker, even though the two systems use different options for that purpose (-v for docker and --bind for singularity).

But maybe it’s not worth the added complexity, and it’s fine to require users wanting an extra binding to know enough about Docker to know which option to use.

@gouttegd
Copy link
Contributor

Random crazy idea: for users who find themselves regularly having to pass options to run.sh, what if we allowed them to have a run.sh.options file which would be sourced (if it exists) at the beginning of run.sh, and which could be used to set the options once and for all?

That file would not be committed (it could be added to the exclusion list in .gitignore), so it would be specifically intended for the user’s local setup. (Settings intended to be applied to all users of the repository would still be specified in the $ONT-odk.yaml file.)

@matentzn
Copy link
Contributor Author

I think this is a great idea.. would make docker options in ODK redundant!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants