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

Validate release date on continuous queues manager #294

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

junpataleta
Copy link
Contributor

This fixes #293

@stronk7
Copy link
Member

stronk7 commented Mar 12, 2024

Thanks @junpataleta , and covered with tests, yay!

It looks perfect, will merge as soon as all checks are done, hopefully green.

Ciao :-)

@stronk7
Copy link
Member

stronk7 commented Mar 12, 2024

As commented via chat, I think that we have to remove the 2 "dry-run" tests. We cannot test that because they require the tracker to be available (for querying). And it's not, so they are failing, and bats is warning us about that (because they don't have any assert_success or assert_failure.

So, the 2 solutions are:

  1. or we remove the dry-run tests.
  2. or we add assert_failure to them, because they are going to fail yes-or-yes (I imagine).

Personally, I'd remove them, because we are not going to be able to test the script without a real tracker, so no query or update can be tried ever.

Ciao :-)

@junpataleta junpataleta force-pushed the validateContinuousReleaseDate branch 4 times, most recently from 07591e5 to 015997d Compare March 12, 2024 13:22
@junpataleta
Copy link
Contributor Author

Thanks, @stronk7. I went with the approach of moving the parameter validation to its own function so it can be tested separately. I tidied up the patch. Please have another look.

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.

We are 99% there, great refactor and tests! I've added a couple of comments in the patch...

@junpataleta junpataleta force-pushed the validateContinuousReleaseDate branch from 015997d to 07baa8f Compare March 13, 2024 02:35
Before performing the job tasks, we need to make sure that the release
date is properly configured. This can be simply done by making sure
that the current date is not well past the end of the on-sync date,
which is calculated by adding 4 weeks to the configured release date.
(Normally, on-sync ends 2 weeks after the release, but it's set to 4
weeks in this patch just in case we have an unusually longer on-sync
period.)

This commit also moves the parameter validation part to its own
function, so it can be tested on its own.

Additional validation to ensure that the `lastweekdate` is not set
after the `releasedate` has also been added.
@junpataleta junpataleta force-pushed the validateContinuousReleaseDate branch from 07baa8f to fc09900 Compare March 13, 2024 02:58
@junpataleta
Copy link
Contributor Author

Thanks for your patience to a bash/bats naab like me, @stronk7 😅. I have made the changes you mentioned.

I also added validation to ensure that lastweekdate is not after the releasedate.

I hope this is good to go.

@stronk7
Copy link
Member

stronk7 commented Mar 13, 2024

Thanks, @junpataleta both for the PR and your patience! Hope it has been a good experience, heh!

About to merge this now, yay!

PS: We still have to discuss if we want, before enabling this script, to have a job to do both:

  • Move IR issues from candidates to current (right now done by the TR- Move awaiting... and documented).
  • Avoid CLR issues, that don't have the candidates/current split to be held (this time we have done it adding the unheld standard comment). There isn't a job for this, neither it's documented.

Notes:

  • It can be a new job only for the CLR issues, or we can join both actions together.
  • If the "adding the unheld standard comment" is not elegant solution, we can always create some field or add a more specific comment (sort of "This issue made the freeze while sitting in the CLR queues" or so). Then we can edit the filters to also avoid issues with that other comment.

Just using this PR to have my thoughts shared, heh.

Ciao :-)

@stronk7 stronk7 merged commit ca456fc into moodlehq:main Mar 13, 2024
4 checks passed
@junpataleta
Copy link
Contributor Author

Yay! Thanks, @stronk7!

I think that the the continuous queues manager should be handling this task of holding or keeping waiting for CLR issues.

About preventing CLR issues from being held, would it be possible to get the date when the waiting for CLR issue has been sent to integration? For example, fetch the date when the issue's status changed from Peer review in progress to Waiting for integration.

Then if it's determined that the date/time the issue was sent to integration falls before the code freeze, it will not be held. Otherwise, it will be held by the continuous queues manager.

What do you think?

@junpataleta junpataleta deleted the validateContinuousReleaseDate branch March 14, 2024 03:06
@stronk7
Copy link
Member

stronk7 commented Mar 14, 2024

After a chat with Jun, we have agreed about:

  • Right now, the continuous queue manager already manages the CLR queue ok (in fact it held a lot of new features/improvements, heh).
  • The critical point is, exclusively, before enabling the continuous queues manager, about how to instruct it that a CLR issue did the freeze (or no), in order to ignore issues that did it.
  • For comparison, with IR issues, we have the "Current in integration" field, that is that we use to differentiate which issues can be held. But we don't have that for CLR issues.
  • So we have to "invent" or "use" something, to be executed before enabling the manager in order to ensure that it's able to detect which CLR issues have made the freeze (to avoid holding them).
  • For 4.4 freeze we have added the official "unheld" message to work-around the problem (the manager skips any issue with that comment), but that's not correct, because those issues shouldn't have been held.

Current proposals are about to:

  • Add a new condition, date based, to select CLR issues. It could be something (untested) like:
AND status WAS NOT ("Waiting for CLR") BEFORE "<<freeze_exact_date_and_time>>"
  • Add a new, more accurate, official comment like "This CLR issue did the freeze and must be considered....". And then make the queues manager to ignore any issue having that comment.

So, next:

  • I will explore the 2 current proposals. For some reason I find the 1st one harder to implement (and easier to forget to configure correctly).
  • Will create a new job, call it "TR - Before enabling the continuous manager" that will do BOTH: What the current "Move issues to current" job does for IR issues AND whatever we plan as solution for the CLR issues.
  • Amend the docs to ensure that the new job is executed before enabling the continuous queues manager.

Ciao :-)

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.

Validate release date before performing actions on the TR - Manage queues on continuous job
2 participants