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

Enable merge queue #177

Merged
merged 3 commits into from
Feb 4, 2025
Merged

Enable merge queue #177

merged 3 commits into from
Feb 4, 2025

Conversation

adam-cattermole
Copy link
Member

Being tracked as part of Kuadrant/kuadrant#7

@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.47%. Comparing base (92de465) to head (a1174c8).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #177      +/-   ##
==========================================
- Coverage   84.04%   82.47%   -1.58%     
==========================================
  Files          18       18              
  Lines        1210     1210              
==========================================
- Hits         1017      998      -19     
- Misses        147      160      +13     
- Partials       46       52       +6     
Flag Coverage Δ
integration 76.03% <ø> (-2.40%) ⬇️
unit 64.23% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
api/v1alpha1 (u) 100.00% <ø> (ø)
pkg/helpers (u) 83.78% <ø> (ø)
pkg/log (u) 93.18% <ø> (ø)
pkg/reconcilers (u) 73.26% <ø> (ø)
pkg/limitador (u) 97.65% <ø> (ø)
controllers (i) 71.24% <ø> (-4.84%) ⬇️
pkg/upgrades ∅ <ø> (∅)

see 2 files with indirect coverage changes

@laurafitzgerald
Copy link
Contributor

Hi @adam-cattermole the change looks good here. I wonder if it would be worth adding the checks to not run some tests on prs that only have tests that don't require changes, e.g. docs. Is there a need for that here that hasn't been accounted for as yet? I don't know the repo well enough to say or no but others might chime in.

@adam-cattermole
Copy link
Member Author

For now I've opted to retain the current functionality but with the merge queue enabled - I think it would be possible to separate out several of the tests into different workflows and apply path filters. That being said it would be easy enough to skip on docs only changes so I'll add a commit with the pre-job

@laurafitzgerald
Copy link
Contributor

For now I've opted to retain the current functionality but with the merge queue enabled - I think it would be possible to separate out several of the tests into different workflows and apply path filters. That being said it would be easy enough to skip on docs only changes so I'll add a commit with the pre-job

Thanks @adam-cattermole that's probably sufficient for now. the concept of the pre-job is that it can be added to/used flexibly over time, so even having it there will give someone the option in the future to go more fine grained. 👍

Signed-off-by: Adam Cattermole <[email protected]>
@adam-cattermole
Copy link
Member Author

Oh wait, I've just realised this workflow runs on a schedule, I'm wondering how the skip-check action works in this case, where there is no change in the paths_ignore.. but also no changes at all

Signed-off-by: Adam Cattermole <[email protected]>
@adam-cattermole
Copy link
Member Author

Added a commit where I hope we'll still run the jobs on schedule

@laurafitzgerald
Copy link
Contributor

Added a commit where I hope we'll still run the jobs on schedule

Interesting. We didn't come across this before on other components. Would you mind adding a note to the issue to include this if the tests are also triggered by .on.schedule

@adam-cattermole
Copy link
Member Author

adam-cattermole commented Feb 4, 2025

Interesting. We didn't come across this before on other components. Would you mind adding a note to the issue to include this if the tests are also triggered by .on.schedule

Sure once this is merged I'll monitor the next scheduled overnight run to double-check it's working as intended - if so I'll document

@adam-cattermole adam-cattermole merged commit 2a56ba8 into main Feb 4, 2025
19 checks passed
@adam-cattermole adam-cattermole deleted the merge-queue branch February 4, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants