-
Notifications
You must be signed in to change notification settings - Fork 5
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
Added support and tests on RHEL systems #18
Conversation
c5dc8bd
to
b6220e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some questions but I guess this is anyhow work in progress
I'll look at the travis logs as well now
df7683d
to
437ac3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments looking through...
f2284fc
to
c7b01d8
Compare
30ff2d2
to
9c16191
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot @espenfl - I think this generalization of the role is very useful and I hope it will help admins installing AiiDA going forward - we'll just need to get people to use it!
Most of my requests are minor.
There's of course the general issue that these pgrep
checks for running processes are not resilient and should be replaced by alternatives where possible. Anyhow, we can open an issue for that and do it later.
tasks/aiida-prepare.yml
Outdated
pip: | ||
name: | ||
- pip~=19.3 | ||
- numpy==1.16.4 | ||
# Flask is required for the AiiDA REST API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be taken care of by installing the rest
extra of the AiIDA package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow it did not. Removing now and then we will see how it goes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at where the git repo is installed, it seems to me we're simply not installing the rest
extra
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, tests are running fine now so that was probably added when one of the plugins was enabled. Maybe. By the way do we want to have the plugin installation enabled by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see... I'm fine with disabling it by default.
This aiida role really started as a role to install the aiida environment for the Quantum Mobile. Eventually I might move out the plugins/codes/examples to a separate role.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as it is in this role, however, we should keep testing it (i.e. please keep it in the molecule playbook)
531ae46
to
ca144d9
Compare
Not sure why that was marked as resolved. Should we not just update the cache on every trigger of
Yes I suspect so, but have not actually checked. |
ca144d9
to
35768ad
Compare
Thanks for your work @espenfl - let me know once you think it's ready to be merged. |
Happy to help. Let us merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's go!
What did I say about the apt_cache ;-) Anyhow, I'll fix this as well as the rest extra |
Great. Did not consider that ;) In fact this is an example of tweaking to pass a test which yields a behavior you not necessarily |
I'm not sure that is true. Note that even if new versions of a package appear in between ansible runs, our role will actually not update them (default of If I understand the ansible philosophy correctly, then in the "strict interpretation" |
Good points. |
fixes #14
fixes #16
Added support and tests on RHEL systems