Add the privileged extension #132
Conversation
Signed-off-by: Gaël Écorchard <gael.ecorchard@cvut.cz>
setup.py
Outdated
| @@ -31,30 +31,31 @@ | |||
|
|
|||
| kwargs = { | |||
| 'name': 'rocker', | |||
| 'version': '0.2.3', | |||
| 'version': '0.3.0', | |||
There was a problem hiding this comment.
please don't change the version number in a patch. This will be done at release time.
There was a problem hiding this comment.
My point of view on this is that we add a functionality and thus must increment the minor version number but will obey to your request. I'll increment first RP #130.
There was a problem hiding this comment.
Yes, that's the policy for semantic versioning. But this commit is not the release. There may be multiple additions that go into the next release. And I couldn't merge this PR at the same time as your other one because this change would conflict.
There was a problem hiding this comment.
I get it with the version number. I actually never contributed to a release-based repository. How do you then manage the version change? Do contributors have to notify somewhere that they did a change that provokes a version change?
setup.py
Outdated
| 'packages': ['rocker'], | ||
| 'package_dir': {'': 'src'}, | ||
| 'package_data': {'rocker': ['templates/*.em']}, | ||
| 'entry_points': { | ||
| 'console_scripts': [ | ||
| 'rocker = rocker.cli:main', | ||
| 'detect_docker_image_os = rocker.cli:detect_image_os', | ||
| ], | ||
| ], |
There was a problem hiding this comment.
please avoid unrelated changes to the PR.
There was a problem hiding this comment.
A little cleanup can be done, but it should be at least separated to a different commit.
src/rocker/extensions.py
Outdated
| return '' | ||
|
|
||
| def get_docker_args(self, cli_args): | ||
| return '--privileged' |
There was a problem hiding this comment.
This needs to be prepended with whitespace to make sure not to run into a preceding argument if not first.
| return '--privileged' | |
| return ' --privileged' |
They are currently string appended
Line 257 in 4f9f6d7
There was a problem hiding this comment.
Wouldn't it be better to actually let extensions return a list of strings and let core.py manage formatting the command, if needed?
There was a problem hiding this comment.
I think a single string at least with core.py inserting the space. "If needed" is not clearly resolved if I return multiple.
There was a problem hiding this comment.
There might be a few words missing in these last sentences and I don't get their sense. Anyway, with "if needed", I meant that subprocess.run and partially pexcpect.spawn accept a list of strings as argument to launch the command.
- Add a space before the argument in the case that the option is not first. - Increment patch version rather than minor (i.e. partially revert previous commit). - Revert unrelated changes compared to master. Signed-off-by: Gaël Écorchard <gael.ecorchard@cvut.cz>
Signed-off-by: Gaël Écorchard <gael.ecorchard@cvut.cz>
|
Reverted the version number and unrelated changes. Should I squeeze commits? |
|
Coverage isn't reporting directly: 100% diff coverage now https://app.codecov.io/gh/osrf/rocker/compare/132/overview |
* Add the privileged extension Signed-off-by: Gaël Écorchard <gael.ecorchard@cvut.cz> Co-authored-by: Tully Foote <tfoote@osrfoundation.org>
Signed-off-by: Gaël Écorchard gael.ecorchard@cvut.cz