Skip to content

Conversation

@romanc
Copy link
Collaborator

@romanc romanc commented Jul 22, 2025

Description

We don't use a merge queue and we allow to merge outdated branches for practical reasons (because in particular pyFV3 and pace tests take a long time to run). It could thus happen that we get a bad merge. In that unlikely case, we'd like to know and thus run tests when pushing to protected branches (main and develop) as a safety net.

How Has This Been Tested?

CI would complain if the yaml formatting was off. We only know for sure that this works once this PR is merged.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation: N/A
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules: N/A
  • New check tests, if applicable, are included

We don't use a merge queue and we allow to merge outdated branches for
practical reasons (because in particular pyFV3 and pace tests take a
long time to run). It could thus happen that we get a bad merge. In that
unlikely case, we'd like to know and thus run tests when pushing to
protected branches (`main` and `develop`) as a safety net.
@FlorianDeconinck
Copy link
Collaborator

SPCL have been using the GH merge queue, but it can require a lot of hand holding apparently :/

@romanc
Copy link
Collaborator Author

romanc commented Jul 22, 2025

SPCL have been using the GH merge queue, but it can require a lot of hand holding apparently :/

Merge queues work great, if you have a stable test environment. What happens when you have an unstable test environment with a merge queue is that sometimes/randomly jobs will drop out of the merge queue on failing tests. And then you have to re-submit the job to the merge queue. This defeats one point of a merge queue (the one that automates parts of the merge pipeline). DaCe doesn't have a stable test environment (they know it) and thus they suffer.

My feeling is that we - on the other hand - have a stable test environment. There are no tests randomly not passing - it just sometimes takes long until all tests pass. Imo this is the perfect fit for a merge queue. I can't configure merge queues, so I went with the next best. Maybe we should discuss merge queues in one of the upcoming NASA/NOAA syncs.

@FlorianDeconinck
Copy link
Collaborator

SPCL have been using the GH merge queue, but it can require a lot of hand holding apparently :/

Merge queues work great, if you have a stable test environment. What happens when you have an unstable test environment with a merge queue is that sometimes/randomly jobs will drop out of the merge queue on failing tests. And then you have to re-submit the job to the merge queue. This defeats one point of a merge queue (the one that automates parts of the merge pipeline). DaCe doesn't have a stable test environment (they know it) and thus they suffer.

My feeling is that we - on the other hand - have a stable test environment. There are no tests randomly not passing - it just sometimes takes long until all tests pass. Imo this is the perfect fit for a merge queue. I can't configure merge queues, so I went with the next best. Maybe we should discuss merge queues in one of the upcoming NASA/NOAA syncs.

Good points. We do have more "it passes" than anything really, so we are on the other side of that bridge.

Bring it on to the NASA/NOAA sync

@romanc romanc requested a review from bensonr July 22, 2025 16:15
@romanc
Copy link
Collaborator Author

romanc commented Jul 22, 2025

@bensonr / @fmalatino I'm also fine putting this on hold until we had time to discuss it at a NOAA/NASA sync. No pressure here.

@romanc
Copy link
Collaborator Author

romanc commented Jul 24, 2025

I just realized: One more reason to start running tests on develop (either by means of a Merge Queue or as proposed in this PR) is that caches currently don't really work.

GitHub Actions can restore caches from the main branch (develop in our case) and pull requests can restore caches from the target branch that they are supposed to merge into, as described in the docs here. Now, since we currently never run actions on the main branch, caches are severely limited. They only work in the scope of one branch (e.g. the first run of the branch creates the cache and subsequent runs of the same PR then can make use of that cache. You can see here that we have multiple caches for the test data (all for different PR numbers).

@romanc
Copy link
Collaborator Author

romanc commented Aug 8, 2025

In the today's pace meeting, we decided to use merge queues directly. I'm closing this PR as superseded by #194.

@romanc romanc closed this Aug 8, 2025
@romanc romanc deleted the romanc/test-on-merge-to-protected-branches branch August 8, 2025 15:54
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