Skip to content

Conversation

@junpataleta
Copy link
Contributor

@junpataleta junpataleta commented Apr 7, 2025

This PR addresses two issues:

  • Because security issues are not treated as "important," they are not automatically moved to the current queue. Thus, they get held, especially during the last week before the release, which is incorrect because security issues are considered related to the release. We must again treat security issues in the continuous queues manager as important.
  • Must-fix and mdlqa issues in CLR are being incorrectly held because the query in run_A3b for the CLR part (filter=23329) does not exclude must-fix and mdlqa issues from being held for issues in the CLR queue.

Just like must-fix and mdlqa issues, security issues are considered
related to the release, so they must be moved to the current queue
just like before.
Adding conditions for the CLR part (filter=23329) of the A3b script so
that mdlqa and must-fixes don't get held, especially during the last
week before the release.

The conditions are not applied to the ones in IR (filter=14000) because
these are already being properly handled in A1 and A2 functions.
@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 31.75%. Comparing base (9e3dfd2) to head (0fc3deb).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #337   +/-   ##
=======================================
  Coverage   31.75%   31.75%           
=======================================
  Files          71       71           
  Lines        3222     3228    +6     
=======================================
+ Hits         1023     1025    +2     
- Misses       2199     2203    +4     
Flag Coverage Δ
check_upgrade_savepoints 0.46% <ø> (-0.01%) ⬇️
checkstyle_manipulations 0.00% <ø> (ø)
compare_databases 2.38% <ø> (-0.01%) ⬇️
continuous_manage_queues_validation 0.71% <ø> (-0.01%) ⬇️
define_excluded 1.11% <ø> (-0.01%) ⬇️
detect_conflicts 1.48% <ø> (-0.01%) ⬇️
diff_extract_changes 0.00% <ø> (ø)
git_sync_two_branches 2.04% <ø> (-0.01%) ⬇️
grunt_process 3.00% <ø> (-0.01%) ⬇️
illegal_whitespace 2.63% <ø> (-0.01%) ⬇️
list_valid_components 0.00% <ø> (ø)
mustache_lint 2.81% <ø> (-0.01%) ⬇️
mustache_lint_plugins 1.30% <ø> (-0.01%) ⬇️
php_lint 1.20% <ø> (-0.01%) ⬇️
prepare_npm_stuff 1.51% <ø> (-0.01%) ⬇️
remote_branch_checker 17.41% <ø> (+0.02%) ⬆️
remote_branch_reporter 0.00% <ø> (ø)
setup 0.00% <ø> (ø)
thirdparty_check 1.45% <ø> (-0.01%) ⬇️
travis-branch-checker 0.00% <ø> (ø)
upgrade_external_backup 1.61% <ø> (-0.01%) ⬇️
verifications 0.00% <ø> (ø)
verify_commit_messages 2.63% <ø> (-0.01%) ⬇️
versions_check_set 5.01% <ø> (-0.01%) ⬇️

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

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@stronk7
Copy link
Member

stronk7 commented Apr 7, 2025

Hi, nice ping-pong we have here,

just for a bit of history, we used to consider security issues as important and move them. But it was decided that we shouldn't because the "important" classification was too much arbitrary, and manual handling was ok (sort of summary of the issue).

See https://tracker.moodle.org/browse/MDLSITE-7296 for reference.

Then, in 76ac0e7, we detected that some of the automatisms were still needed and we re-introduced them (partially). The security flagged issues was one of the conditions that was left out. Again, the same MDLSITE has it documented.

So, now, some is/are being re-introduced.

I just would suggest to compare it with how it was working originally (see the commit), mainly to see if there is any other condition to check (we were also checking privacy or testing related issues, for example).

Said that, adding back the level condition (aka security), looks ok to me.

Ciao :-)

@stronk7
Copy link
Member

stronk7 commented Apr 7, 2025

And, about the 2nd part of the issue... unless I'm wrong... A2 happens before any A3, so the conditions shouldn't be needed there is the previous point already has moved the issues.

I think that, historically, that's the reason of A3b not having extra conditions, because any move should have happened already (in A2) and all the remaining issues are candidates to be held.

Ciao :-)

PS: Take this with a pinch of salt, it's just a theory from my memories, please check.

Adding conditions for the CLR part (filter=23329) of the A3b script so
that mdlqa and must-fixes don't get held, especially during the last
week before the release.

The conditions are not applied to the ones in IR (filter=14000) because
these are already being properly handled in A1 and A2 functions.
@junpataleta junpataleta force-pushed the PreventIncorrectHolds branch from 0f449a0 to 0fc3deb Compare April 8, 2025 03:29
@junpataleta
Copy link
Contributor Author

Thanks, Eloy. Yeah, however, A2 does not handle issues in CLR already, so A3b will hold the must-fix and mdlqa CLR issues as well.

Regarding adding back the additional components (criterion e), the components Privacy, Automated tests (Behat), and PHPUnit are now normally under CLR conditions. So, adding them back to A2 means that they will be moved to the current queue if the CLR/IR triage job has not got to them first. Adding them to A3b will prevent them from being held, but they can get stuck in the IR candidates queue, especially if there are no CLRers available.

I think we should reconsider adding criterion e on a separate issue.

Copy link
Member

@stronk7 stronk7 left a comment

Choose a reason for hiding this comment

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

All ok, for me, then. Good job!

@junpataleta
Copy link
Contributor Author

Awesome. Thanks, Eloy! Self-merging this!

@junpataleta junpataleta merged commit 31cfd83 into moodlehq:main Apr 8, 2025
48 of 51 checks passed
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.

3 participants