furnace/cli.py: add command line interface to furnace #5
Conversation
kocsob
changed the title
| super().__init__(source, destination) | ||
| self.read_only = read_only | ||
|
|
||
| def get_mount_parameters(self): |
There was a problem hiding this comment.
is get_mount_parameters unused now? Maybe it would need to be called from mount() below?
There was a problem hiding this comment.
Is this commit related to the CLI related PR?
There was a problem hiding this comment.
is get_mount_parameters unused now?
No, it is called by the base MountContext, which uses this parameters. There is an unfortunate situation, that for the read-only bind mount requires two mount system call.
badicsalex
left a comment
badicsalex
left a comment
There was a problem hiding this comment.
Only merge after #4?
| if args.persistent: | ||
| run_container(args.root_dir, args.volume, args.isolate_networking, args.hostname, args.cmd) | ||
| else: | ||
| with tempfile.TemporaryDirectory( |
| super().__init__(source, destination) | ||
| self.read_only = read_only | ||
|
|
||
| def get_mount_parameters(self): |
kocsob
force-pushed
the
add-cli-interface
branch
from
5ee71e7 to
502bd42
Compare
furnace/cli.py
Outdated
| if readwrite == 'rw': | ||
| readonly = False | ||
|
|
||
| if destination.startswith('/'): |
There was a problem hiding this comment.
I would "cast" destination to a Path object sooner, so destination.is_absolute() could be called here, which is more meaningful.
furnace/cli.py
Outdated
| return parser.parse_args(argv[1:]) | ||
|
|
||
|
|
||
| def create_bind_mount_list(volumes): |
There was a problem hiding this comment.
A unit test would be nice for this function.
furnace/cli.py
Outdated
| return parser.parse_args(argv[1:]) | ||
|
|
||
|
|
||
| def create_bind_mount_list(volumes): |
There was a problem hiding this comment.
It would be more user-friendly if this method would raise a meaningful error message in case of invalid values, and should be more strict if
E.g. it silently ignores "/host/path:/container/path:asdf" and makes the mount read-only, but raises ValueError for "/host/path" It should say something specific about the problem with this parameter and exit non-zero without a traceback.
furnace/cli.py
Outdated
| help="this command that will be run. If it is empty, than furnace will give an interactive shell" | ||
| ) | ||
| parser.add_argument( | ||
| '--version', |
There was a problem hiding this comment.
-V is common for linux tools for version alias
furnace/cli.py
Outdated
| parser.add_argument( | ||
| 'cmd', | ||
| nargs='*', | ||
| help="this command that will be run. If it is empty, than furnace will give an interactive shell" |
There was a problem hiding this comment.
I would reprase this a bit: "the command that will be run. If empty, furnace will drop into an interactive shell"
furnace/cli.py
Outdated
| version='%(prog)s {version}'.format(version=version.get_version()), | ||
| ) | ||
| parser.add_argument( | ||
| '--hostname', |
furnace/cli.py
Outdated
| parser.add_argument( | ||
| '-v', '--volume', | ||
| action='append', | ||
| metavar='src:dst:rw', |
There was a problem hiding this comment.
I would add the dest parameter here (e.g. volumes), because it's confusing later that args.volume contains a list, not a single value.
kocsob
force-pushed
the
add-cli-interface
branch
3 times, most recently
from
72ff252 to
da9c0d8
Compare
kocsob
force-pushed
the
add-cli-interface
branch
from
da9c0d8 to
ab71880
Compare
|
This pull request introduces 1 alert when merging ab71880 into 309bd07 - view on LGTM.com new alerts:
|
kocsob
force-pushed
the
add-cli-interface
branch
from
ab71880 to
4369c3b
Compare
No description provided.