Introduce command to stop local containers #157
Conversation
invenio_cli/cli.py
Outdated
| @click.option('--stop', default=False, is_flag=True, | ||
| help='Stop containers') | ||
| def run(stop): |
There was a problem hiding this comment.
After discussing IRL, the stop should be a separate command, not a flag on the run. (We we called option b when we talked)
| @click.option('--stop', default=False, is_flag=True, | |
| help='Stop containers') | |
| def run(stop): | |
| def run(): |
invenio_cli/helpers/commands.py
Outdated
| """Run development server and celery queue.""" | ||
| if stop: | ||
| command = [ |
There was a problem hiding this comment.
There is a stop_containers function in the docker_helper file. It might be good using that :)
tests/test_helpers/test_commands.py
Outdated
| @@ -295,7 +295,7 @@ def test_localcommands_run( | |||
| fake_cli_config): | |||
| commands = LocalCommands(fake_cli_config) | |||
|
|
|||
| commands.run() | |||
| commands.run(stop=False) | |||
There was a problem hiding this comment.
Not re-reviewing tests. Not updated as mentioned IRL.
invenio_cli/helpers/commands.py
Outdated
| @@ -228,6 +228,15 @@ def demo(self): | |||
| command = ['pipenv', 'run', 'invenio', 'rdm-records', 'demo'] | |||
| subprocess.run(command, check=True) | |||
|
|
|||
| def stop(self): | |||
| """Stops containers.""" | |||
| docker_helper = DockerHelper( | |||
There was a problem hiding this comment.
LGTM. This is more a comment for future work:
We should try to find a common way of using docker_helper. The containerize command has an instance as attribute and the local commands instantiate it each time is needed. It might be able to be refactored into something common.
invenio_cli/helpers/commands.py
Outdated
| local=True) | ||
|
|
||
| docker_helper.stop_containers() | ||
| click.secho('Stopped containers...', fg='green') |
There was a problem hiding this comment.
[Full OCD]: IMO the ... are sort of an indication of something going on - in progress -. Therefore, I would use if for the messages before the action. e.g. "creating indexes...".
For messages of things that are already finished I would not put the ... For example
invenio-cli/invenio_cli/helpers/commands.py
Line 427 in 28a2e17
If you decide to change this (totally up to yo, as in, this is my OCD). Could you change the Destroyed ... messages too and remove the ...
| click.secho('Stopped containers...', fg='green') | |
| click.secho('Containers stopped', fg='green') |
There was a problem hiding this comment.
Yes I'll do that :)
tests/test_helpers/test_commands.py
Outdated
| patched_docker_helper, patched_subprocess, fake_cli_config): | ||
| commands = LocalCommands(fake_cli_config) | ||
|
|
||
| # Needed to fake call to docker-compose --version but not call |
There was a problem hiding this comment.
Doubt: What does this comment applies to?
There was a problem hiding this comment.
This comment was made when developing the test_localcommands_destroy and I can't really remember the reason why was done.
tests/test_helpers/test_commands.py
Outdated
| @@ -312,6 +312,28 @@ def test_localcommands_run( | |||
| assert patched_subprocess.Popen.mock_calls == expected_calls | |||
|
|
|||
|
|
|||
| @patch('invenio_cli.helpers.commands.subprocess') | |||
| @patch('invenio_cli.helpers.docker_helper.subprocess') | |||
| def test_localcommands_stop( | |||
There was a problem hiding this comment.
moderate: There is no assertion in this test, in case something does not work at desired what kind of feedback do we get when running tests? How is it notified? stacktrace?
There was a problem hiding this comment.
The new commit addressed the missing assertion 6380144
invenio_cli/helpers/docker_helper.py
Outdated
| def stop_containers(self): | ||
| """Stop currently running containers.""" | ||
| command = ['docker-compose', | ||
| '--file', 'docker-compose.full.yml', 'stop'] | ||
|
|
||
| if self.local: | ||
| command[2] = 'docker-compose.yml' | ||
| command = [ | ||
| 'docker-compose', | ||
| '--file', | ||
| 'docker-compose.yml' if self.local else 'docker-compose.full.yml', | ||
| 'stop' | ||
| ] | ||
|
|
||
| subprocess.call(command) |
There was a problem hiding this comment.
Turns out the same trick @lnielsen mentioned can be used for the stop command too (It works. I learned something!):
def stop_containers(self):
"""Stop currently running containers."""
command = [
'docker-compose',
'--file',
'docker-compose.full.yml',
'stop'
]
subprocess.call(command)Testing becomes much easier and uniform too.
| - REQUIREMENTS=devel | ||
|
|
||
| python: | ||
| - "3.6" | ||
| # Lower than 3.6.9 distutils imports `imp` and throws a deprecation |
There was a problem hiding this comment.
Is this the case for this library?
|
The successor of this PR is #167 |
No description provided.