Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow verdi status to skip checks #4181

Merged
merged 8 commits into from
Jun 18, 2020
Merged

Conversation

chrisjsewell
Copy link
Member

This PR adds flags to verdi status --no-rmq --no-daemon.
My specific use case for this is, when initiating a Docker container, I want to wait for the environment to finished initialising before starting a daemon by polling the exit code of verdi status.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jun 17, 2020

Well thats annoying, this all run fine locally
(re-building local environment at least fixed the pre-commit)

@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

Merging #4181 into develop will increase coverage by 0.01%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4181      +/-   ##
===========================================
+ Coverage    78.89%   78.89%   +0.01%     
===========================================
  Files          467      467              
  Lines        34500    34503       +3     
===========================================
+ Hits         27215    27218       +3     
  Misses        7285     7285              
Flag Coverage Δ
#django 70.82% <75.00%> (+0.01%) ⬆️
#sqlalchemy 71.70% <75.00%> (+0.01%) ⬆️
Impacted Files Coverage Δ
aiida/cmdline/commands/cmd_status.py 77.34% <75.00%> (+0.95%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94a40ae...5a7660f. Read the comment docs.

@sphuber
Copy link
Contributor

sphuber commented Jun 18, 2020

@chrisjsewell why don't you need the status of RabbitMQ but you do need it for Postgres? I can see the point of the daemon. It not running is not really a "failure". So would it maybe make more sense to turn the daemon notification in a warning by default, i.e. swapping out the red cross for a yellow circle or something, and just returning 0. Anyway, verdi daemon status can be used to determine whether the daemon is running. If it doesn't return non-negative exit status if daemon is not running, we can of course add this, which would make sense

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jun 18, 2020

why don't you need the status of RabbitMQ

Well rabbitmq is only required for running computations. If for example you just want to spin up a container to inspect an existing database, you only want to check that it is ready for access.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jun 18, 2020

So would it maybe make more sense to turn the daemon notification in a warning by default

Yep something like that could work; I just would like a consistent/simple way to check if the "AiiDA environment" is either:

  1. ready to access as a database (profile + postgres)
  2. ready to start daemons (profile + postgres + rabbitmq)
  3. ready to start calculations (profile + postgres + rabbitmq + daemons)

@chrisjsewell
Copy link
Member Author

swapping out the red cross for a yellow circle or something

At least for my use case, I don't really care what is printed lol; I'm just using it as an "API"

For example in https://github.com/chrisjsewell/aiida-vcode-ext/runs/781411414?check_suite_focus=true, I'm currently using:

docker exec --user aiida aiida-core timeout 60s bash -c 'until verdi profile list 2> /dev/null; do echo -n .; sleep 1; done'

@sphuber
Copy link
Contributor

sphuber commented Jun 18, 2020

I opened a PR against your branch to get rid of the --no-daemon and just return 0 when it is not running. Also turned the text into a "warning" with a yellow circle instead of a cross, because a daemon that is not running is nowhere near as bad as a failing database connection

The daemon not running is not the same as not being able to connect to
the Postgres database or the RabbitMQ message broker. So it shouldn't
return a non-zero exit code either. If the user wants to poll this, the
command `verdi daemon status` should be used instead.
@chrisjsewell
Copy link
Member Author

Yep thats great cheers, and I like the new pytest fixture 👌

@sphuber
Copy link
Contributor

sphuber commented Jun 18, 2020

Yep thats great cheers, and I like the new pytest fixture ok_hand

Cheers. I added it recently as it simplifies a great deal of test code. Maybe should send a quick email soon to the team to notify everyone of its existence

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go, thanks @chrisjsewell

@sphuber sphuber merged commit 3a5853e into aiidateam:develop Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants