-
Notifications
You must be signed in to change notification settings - Fork 62
Implementation of Docker container options for steps #881
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
Conversation
| "matching-rule": "any", | ||
| "mapping": {"regex;(.+)": {"type": "str"}}, | ||
| }, | ||
| "options": {"type": "map", "allowempty": True}, |
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.
Preferred to keep the yml validation simple as it would be unwieldy to specify all allowable options for all container runtimes
|
|
||
| self._update_with_engine_config(args) | ||
|
|
||
| args.update(step.options) |
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.
after update_with_engine_config to ensure overall configuration is overriden
ivotron
left a comment
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 looks great! The only other thing that it could be added is documentation. Would it be possible to include add a new attribute to this table, with a note specifying that this is only available for Docker.
Also, a couple of sentences can be included in the Custom Engine Configuration section that mention that this can be done at the level of a step.
|
thanks @Jswig !! |
This contains changes allowing to pass step-specific container options to the Docker runner.
Edited some tests to reflect new behavior.