aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
412 stars 184 forks source link

CI: Test with newer RabbitMQ versions #6419

Closed danielhollas closed 1 month ago

danielhollas commented 1 month ago

Version 3.6/3.7 are super old and unsupported, let's test with newer versions instead.

We still want to support v3.8, since that's the last one that doesn't require any extra configuration (i.e. setting longer timeouts)

See also https://aiida.discourse.group/t/supported-rabbitmq-versions/392

If this PR gets accepted, we should also update the docs, which currently says:

RabbitMQ v3.5 and older are [end-of-life](https://www.rabbitmq.com/versions.html) 
and are not supported in any way. For RabbitMQ v3.8.15 and up,
AiiDA is not compatible with the default configuration of the server.
sphuber commented 1 month ago

I am not quite sure what to do with this worflow. I added this back when I added support for more configuration options for the RabbitMQ connection, most notably to support SSL/TLS. Since there were some differences in behavior between v3.5 and v3.6 that I didn't initially uncover, I added these tests as a way to have "some" sense of whether our RMQ use would still be working.

However, since then, I don't think we have ever had these tests fail. Mostly this is because we don't really touch the RMQ behavior in aiida-core, as this is all done by kiwipy and its dependencies. So I am thinking whether we even need to keep them. At the least we should maybe just move them to the nightly run? I don't see a need to run them on every push and PR. I guess then it would also not be so bad to run against many more recent versions.

Thoughts @danielhollas ?

danielhollas commented 1 month ago

I don't have a strong opinion on this tbh. I'd just note that these tests are quite quick.

At the least we should maybe just move them to the nightly run?

Fair. However, based on recent experience, is anybody actually checking the nightly runs? :sweat_smile: Is there an email going somewhere when a nightly run fails? Just checking since we didn't notice that the test-install workflow was failing, but that might have been because GHA is stupid and didn't notify us that the workflow file itself was invalid.

I don't see a need to run them on every push and PR. I guess then it would also not be so bad to run against many more recent versions.

Maybe we could gate these tests based on the path of modified files? e.g. run them only when pyproject.toml changes (e.g. kiwipy update), or files in aiida/manage/ and aiida/brokers/?

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.66%. Comparing base (ef60b66) to head (b5153bc). Report is 10 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6419 +/- ## ========================================== + Coverage 77.51% 77.66% +0.15% ========================================== Files 560 562 +2 Lines 41444 41652 +208 ========================================== + Hits 32120 32343 +223 + Misses 9324 9309 -15 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sphuber commented 1 month ago

Fair. However, based on recent experience, is anybody actually checking the nightly runs? 😅 Is there an email going somewhere when a nightly run fails? Just checking since we didn't notice that the test-install workflow was failing, but that might have been because GHA is stupid and didn't notify us that the workflow file itself was invalid.

Well, for the nightly.yml workflow, I actually added a step where it adds a post to the Slack channel if it fails. As you said though, it was the test-install.yml one that failed recently that has no such step included.

I know that it can send out an email on failure somehow, because old contributor Leopold Talirz was and still is receiving them. However, I have never been able to figure out how he is receiving them. I couldn't find an option/setting anywhere. The current conjecture is that he gets it because he turned on email notifications for failing builds in general.

Maybe we could gate these tests based on the path of modified files? e.g. run them only when pyproject.toml changes (e.g. kiwipy update), or files in aiida/manage/ and aiida/brokers/?

Honestly, there is nothing really in these files that we can change that can all of a sudden break anything. The only real pathway would be a change in kiwipy. But I am also not so worried about us breaking things, but rather just to know whether new versions of RMQ still work.

So I would actually propose to either merge the tests in the rabbitmq.yml workflow into the nightly.yml one, or just make the trigger in rabbitmq.yml a CRON-based one

danielhollas commented 1 month ago

Okay, I've moved the rabbitmq tests to nightly.yml, and only included version >3.11, since those are the only once currently supported. Version 3.8 is tested as part of normal test suite in ci-code.yml.

I know that it can send out an email on failure somehow, because old contributor Leopold Talirz was and still is receiving them.

Are you talking about receiving emails directly from GitHub? I guess if you subscribe to the dev-aiida-core channel on Slack you'll get an email that way right?

sphuber commented 1 month ago

Thanks @danielhollas