You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@liminal.apache.org by GitBox <gi...@apache.org> on 2020/07/26 17:15:31 UTC
[GitHub] [incubator-liminal] naturalett opened a new pull request #1: Adding liminal stop button
naturalett opened a new pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1
Signed-off-by: Lidor Ettinger <li...@gmail.com>
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] naturalett commented on a change in pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
naturalett commented on a change in pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#discussion_r524544571
##########
File path: scripts/liminal
##########
@@ -124,10 +125,17 @@ def start():
if docker_is_running():
# initialize liminal home by default
environment.get_liminal_home()
- result = subprocess.call(
- [f'docker-compose -f "{environment.get_liminal_home()}/docker-compose.yml" up -d'],
- env=os.environ, shell=True)
- click.echo("Open Airflow in your browser. Please point your browser to http://localhost:8080 to access Airflow.")
+ docker_compose_path, project_dir = get_docker_compose_paths()
+ docker_compose_command('call', 'up', ['-d'])
Review comment:
But then the **liminal stop** doesn't really take a role. It will make them to use ctrl+c.
Maybe should I add a flag for this behaviour?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] assapin commented on a change in pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
assapin commented on a change in pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#discussion_r525506885
##########
File path: README.md
##########
@@ -167,12 +167,28 @@ This will rebuild the airlfow docker containers from scratch with a fresh versio
liminal start
```
-5. Navigate to [http://localhost:8080/admin](http://localhost:8080/admin)
+5. Stop the server
+```bash
+liminal stop
+```
+
+6. Display the logs server
Review comment:
server logs, not logs server
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] naturalett commented on a change in pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
naturalett commented on a change in pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#discussion_r524544571
##########
File path: scripts/liminal
##########
@@ -124,10 +125,17 @@ def start():
if docker_is_running():
# initialize liminal home by default
environment.get_liminal_home()
- result = subprocess.call(
- [f'docker-compose -f "{environment.get_liminal_home()}/docker-compose.yml" up -d'],
- env=os.environ, shell=True)
- click.echo("Open Airflow in your browser. Please point your browser to http://localhost:8080 to access Airflow.")
+ docker_compose_path, project_dir = get_docker_compose_paths()
+ docker_compose_command('call', 'up', ['-d'])
Review comment:
But then the **liminal stop** doesn't really take a role. The -d flag will make them to use ctrl+c in order to exit from the running process of the docker compose.
Maybe should I add a flag for this behaviour?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] naturalett commented on a change in pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
naturalett commented on a change in pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#discussion_r525432737
##########
File path: scripts/liminal
##########
@@ -113,7 +124,7 @@ def start():
# initialize liminal home by default
environment.get_liminal_home()
docker_compose_path, project_dir = get_docker_compose_paths()
- docker_compose_command('up', [])
+ docker_compose_command('up', ['-d'])
Review comment:
I added liminal logs --follow/--tail.
As well, I updated the README. Let me know if screenshots are still required.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] aviemzur commented on pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
aviemzur commented on pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#issuecomment-720348512
R: @assapin
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] assapin commented on pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
assapin commented on pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#issuecomment-728766054
@aviemzur feel free to chime in with any inputs
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] naturalett commented on a change in pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
naturalett commented on a change in pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#discussion_r524964527
##########
File path: scripts/liminal
##########
@@ -84,9 +84,10 @@ def deploy_liminal_core_internal(clean):
def docker_compose_command(command_name, args):
docker_compose_path, project_dir = get_docker_compose_paths()
concated_args = ' '.join(args)
- subprocess.call(
+ output = subprocess.run(
Review comment:
Yes the status code and the stderr.
Concerning that scope there's not much use for the return result of the command up.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] assapin merged pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
assapin merged pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] assapin commented on a change in pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
assapin commented on a change in pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#discussion_r524939950
##########
File path: scripts/liminal
##########
@@ -84,9 +84,10 @@ def deploy_liminal_core_internal(clean):
def docker_compose_command(command_name, args):
docker_compose_path, project_dir = get_docker_compose_paths()
concated_args = ' '.join(args)
- subprocess.call(
+ output = subprocess.run(
Review comment:
what happens when you do run on the command "up"? does it return?
##########
File path: scripts/liminal
##########
@@ -113,7 +124,7 @@ def start():
# initialize liminal home by default
environment.get_liminal_home()
docker_compose_path, project_dir = get_docker_compose_paths()
- docker_compose_command('up', [])
+ docker_compose_command('up', ['-d'])
Review comment:
-d means users don't by default see the logs.
it also enables the "stop" command to become available.
there is a question on how users see logs esp. errors.
I don't think it's ok to expect them to do docker ps and docker logs- some of them don't know we're running docker.
I suggest to add liminal logs command which delegates to docker compose logs. including the ability for -f and tail (delegate directly to docker compose).
Also need to update the README with the stop and logs command, ideally also add screenshots of outptus users should expect
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] assapin commented on a change in pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
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
-@cli.command("stop", short_help="stops the docker compose.")
+@cli.command("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:
users@infra.apache.org
[GitHub] [incubator-liminal] assapin commented on a change in pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
assapin commented on a change in pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#discussion_r524556056
##########
File path: scripts/liminal
##########
@@ -124,10 +125,17 @@ def start():
if docker_is_running():
# initialize liminal home by default
environment.get_liminal_home()
- result = subprocess.call(
- [f'docker-compose -f "{environment.get_liminal_home()}/docker-compose.yml" up -d'],
- env=os.environ, shell=True)
- click.echo("Open Airflow in your browser. Please point your browser to http://localhost:8080 to access Airflow.")
+ docker_compose_path, project_dir = get_docker_compose_paths()
+ docker_compose_command('call', 'up', ['-d'])
Review comment:
where is the stdout and err go? how can the user see them?
what feedback do users get that the process is successfully up and running?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] naturalett commented on a change in pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
naturalett commented on a change in pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#discussion_r524542086
##########
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
-@cli.command("stop", short_help="stops the docker compose.")
+@cli.command("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:
Yes
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] assapin commented on a change in pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
assapin commented on a change in pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#discussion_r525510382
##########
File path: scripts/liminal
##########
@@ -84,9 +84,14 @@ def deploy_liminal_core_internal(clean):
def docker_compose_command(command_name, args):
docker_compose_path, project_dir = get_docker_compose_paths()
concated_args = ' '.join(args)
- subprocess.call(
+ if 'follow' in str(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)
+ output = subprocess.run(
Review comment:
better to prepare the command string once, and decide if to invoke it with subprocess.call or run.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] assapin commented on a change in pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
assapin commented on a change in pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#discussion_r525989533
##########
File path: scripts/liminal
##########
@@ -84,9 +84,11 @@ def deploy_liminal_core_internal(clean):
def docker_compose_command(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 'follow' in str(args):
+ subprocess.call(run_command, env=os.environ, shell=True)
+ output = subprocess.run(run_command,env=os.environ, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
Review comment:
else?
##########
File path: scripts/liminal
##########
@@ -104,6 +106,29 @@ def deploy_liminal_apps(path, clean):
target_yml_name = os.path.join(environment.get_dags_dir(), yml_name)
shutil.copyfile(config_file, target_yml_name)
+def liminal_is_running():
+ stdout, stderr = docker_compose_command('ps', [])
+ return "liminal" in stdout
+
+@cli.command("stop", short_help="stops the docker compose")
+def stop():
+ if docker_is_running() and liminal_is_running():
+ # initialize liminal home by default
+ environment.get_liminal_home()
+ docker_compose_command('down', [])
Review comment:
it's enough to write down() and all the containers really shut down including postgres?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] aviemzur removed a comment on pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
aviemzur removed a comment on pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#issuecomment-720348512
R: @assapin
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-liminal] assapin commented on a change in pull request #1: Adding liminal stop button
Posted by GitBox <gi...@apache.org>.
assapin commented on a change in pull request #1:
URL: https://github.com/apache/incubator-liminal/pull/1#discussion_r524488140
##########
File path: scripts/liminal
##########
@@ -124,10 +125,17 @@ def start():
if docker_is_running():
# initialize liminal home by default
environment.get_liminal_home()
- result = subprocess.call(
- [f'docker-compose -f "{environment.get_liminal_home()}/docker-compose.yml" up -d'],
- env=os.environ, shell=True)
- click.echo("Open Airflow in your browser. Please point your browser to http://localhost:8080 to access Airflow.")
+ docker_compose_path, project_dir = get_docker_compose_paths()
+ docker_compose_command('call', 'up', ['-d'])
Review comment:
why -d? for now we need the user to see logs in front of them in case there are issues
----------------------------------------------------------------
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:
users@infra.apache.org