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

CI: Test with newer RabbitMQ versions #6419

Merged
merged 3 commits into from
May 27, 2024
Merged

Conversation

danielhollas
Copy link
Collaborator

@danielhollas danielhollas commented May 25, 2024

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.

@@ -23,7 +23,7 @@ jobs:
strategy:
fail-fast: false
matrix:
rabbitmq-version: ['3.6', '3.7', '3.8']
rabbitmq-version: ['3.8', '3.9', '3.10', '3.11', '3.12', '3.13']
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not saying we should test all these versions, but wanted to run them at least once to make sure they all work.

@sphuber
Copy link
Contributor

sphuber commented May 26, 2024

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
Copy link
Collaborator Author

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? 😅 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/?

Copy link

codecov bot commented May 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sphuber
Copy link
Contributor

sphuber commented May 27, 2024

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

Version 3.6/3.7 are super old and unsupported,
let's test with newer, supported 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), but this version is tested as part of normal tests in ci-code.yml

Since AiiDA interacts with RabbitMQ only via the kiwipy interface,
it's unlikely that changes to AiiDA would break it. Therefore, the
RabbitMQ tests are moved to the nightly run.
@danielhollas
Copy link
Collaborator Author

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 sphuber merged commit b47a566 into aiidateam:main May 27, 2024
20 checks passed
@sphuber
Copy link
Contributor

sphuber commented May 27, 2024

Thanks @danielhollas

@danielhollas danielhollas deleted the rmq-bump branch May 27, 2024 22:18
mikibonacci pushed a commit to mikibonacci/aiida-core that referenced this pull request Sep 3, 2024
The `rabbitmq.yml` was running a subset of the unit test suite against
a number of versions of RabbitMQ. The versions tested were very outdated
and are not officially supported anymore. The versions are updated to
3.11, 3.12, and 3.13.

Since the introduction, the tests have never really failed, which is not
too suprising as the code in `aiida-core` doesn't actually directly
interface with RabbitMQ. Rather this is done in `kiwipy`. Therefore, the
tests are moved to the nightly workflow such that they are no longer
run on every push and PR, which is overkill.
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.

2 participants