-
Notifications
You must be signed in to change notification settings - Fork 192
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
Use GitHub Actions services (for PostgreSQL and RabbitMQ) #3901
Use GitHub Actions services (for PostgreSQL and RabbitMQ) #3901
Conversation
sudo rm -f /etc/apt/sources.list.d/dotnetdev.list /etc/apt/sources.list.d/microsoft-prod.list | ||
sudo apt update | ||
sudo apt install postgresql-10 rabbitmq-server graphviz | ||
sudo systemctl status rabbitmq-server.service | ||
sudo apt install postgresql-10 graphviz |
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.
why do we still need to install postgresql-10 manually here?
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.
Due to the PostgreSQL tests. It may be that we do not need to after @ltalirz has moved these tests out of the package, but I am not sure?
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.
More specifically, all tests using this class will not run properly - at least that was my prior experience when messing around with these workflows adding the, now obsolete, PostgreSQL GitHub Action.
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.
If you are mentioning the moving out of the PGSU
class, then that already happened. The PR was merged yesterday. Did you try to remove it and see if the tests still run? If so what was the failure?
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.
ah that makes sense, because there it relies on pg_test
to create a postgres cluster on the localhost. So if the service just exposes a postgres database on some other machine, then we still have to install it manually
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.
Concerning the removal of postgresql-10
: It is indeed not possible, see here.
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.
In the actions that ran for this PR we again see sporadic failure to install dependencies using apt. Since it seems that we can't get rid of those even with the improvements introduced with this PR, I think we should probably looking into either providing a custom image or caching. Either way, this would probably be out of scope for this PR.
Yeah - there's some weird stuff happening. Hmm, also since the GitHub Pages were having some maintenance, the Docs CI is failing (I'm guessing that's why).
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.
In the actions that ran for this PR we again see sporadic failure to install dependencies using apt. Since it seems that we can't get rid of those even with the improvements introduced with this PR, I think we should probably looking into either providing a custom image or caching. Either way, this would probably be out of scope for this PR.
Since there are "only" 3 packages left (although there may be a lot of dependencies) we could download the packages and keep them under /.ci/
or similar and install them from there? I don't know if this is a viable solution, but it would avoid the fragile sudo apt update
.
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.
Since there are "only" 3 packages left (although there may be a lot of dependencies) we could download the packages and keep them under
/.ci/
or similar and install them from there? I don't know if this is a viable solution, but it would avoid the fragilesudo apt update
.
The aiida-core
repository is already over 100MB, so I would be really hesitant to start vendoring entire system software packages just to make the CI work.
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.
I found this page commenting on the issue. See also actions/runner-images#675
Codecov Report
@@ Coverage Diff @@
## develop #3901 +/- ##
========================================
Coverage 78.00% 78.00%
========================================
Files 457 457
Lines 33708 33708
========================================
Hits 26295 26295
Misses 7413 7413
Continue to review full report at Codecov.
|
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.
LGTM!
9bdd043
to
d863518
Compare
Thanks! But just removed the testing commit that removed installing |
2dcc18d
to
d863518
Compare
Move over to a more "clean" setup of system services in CI tests, here PostgreSQL and RabbitMQ.
This utilizes GitHub Actions services, specifically integrating the Docker images for PostgreSQL and RabbitMQ.
Hence, some oblique lines in the workflow can be removed under the "Install system dependencies" step(s) as well as the PostgreSQL action.
Note: This does not solve the issue of having a PostgreSQL super user with an empty password; the authentication method is kept to
"trust"
, i.e., allowing all connections no matter the database user password.