assapin commented on a change in pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#discussion_r524494039
##########
File path: scripts/liminal
##########
@@ -75,18 +75,21 @@ def deploy_liminal_core_internal(clean):
print(f'Liminal was installed locally, changing the LIMINAL_VERSION
parameter to {full_path}')
os.environ[environment.LIMINAL_VERSION_PARAM_NAME]= full_path
if clean:
- docker_compose_command('down', ['--remove-orphans','--rmi', 'local'])
- docker_compose_command('build', ['--no-cache'])
- docker_compose_command('run', ['webserver', 'resetdb', '-y'])
- docker_compose_command('run', ['webserver','initdb'])
- docker_compose_command('down', ['--remove-orphans'])
+ docker_compose_command('call', 'down', ['--remove-orphans','--rmi',
'local'])
+ docker_compose_command('call', 'build', ['--no-cache'])
+ docker_compose_command('call', 'run', ['webserver', 'resetdb', '-y'])
+ docker_compose_command('call', 'run', ['webserver','initdb'])
+ docker_compose_command('call', 'down', ['--remove-orphans'])
-def docker_compose_command(command_name, args):
+def docker_compose_command(process, command_name, args):
docker_compose_path, project_dir = get_docker_compose_paths()
concated_args = ' '.join(args)
- subprocess.call(
- [f'docker-compose -f "{docker_compose_path}" -p liminal
--project-directory {project_dir} {command_name} {concated_args}'],
- env=os.environ, shell=True)
+ run_command = [f'docker-compose -f "{docker_compose_path}" -p liminal
--project-directory {project_dir} {command_name} {concated_args}']
+ if 'call' in process:
Review comment:
please use subprocess.run
which returns a CompletedProcess object with both std out, std err and
return value.
for the one place which needs it (checking if liminal is running), you can
retrieve STDOUT from this object and check if it contains liminal.
other invocations of this method anyhow don't use the retrun value
##########
File path: scripts/liminal
##########
@@ -105,17 +108,15 @@ def deploy_liminal_apps(path, clean):
shutil.copyfile(config_file, target_yml_name)
def is_airflow_running():
Review comment:
please rename to liminal_is_running (since you are checking for liminal,
not airflow)
##########
File path: scripts/liminal
##########
@@ -105,17 +108,15 @@ def deploy_liminal_apps(path, clean):
shutil.copyfile(config_file, target_yml_name)
def is_airflow_running():
- result = subprocess.check_output([f'docker-compose -f
"{environment.get_liminal_home()}/docker-compose.yml" ps'], shell=True,
universal_newlines=True)
+ result = docker_compose_command('check_output', 'ps', [])
return "liminal" in result
[email protected]("stop", short_help="stops the docker compose.")
[email protected]("stop", short_help="stops the docker compose")
def stop():
-
if docker_is_running() and is_airflow_running():
# initialize liminal home by default
environment.get_liminal_home()
- result = subprocess.call([f'docker-compose -f
"{environment.get_liminal_home()}/docker-compose.yml" down'],
- env=os.environ, shell=True)
+ docker_compose_command('call', 'down', [])
Review comment:
did you check if it kills all the containers after down?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
