From 95ad7d6201fa89459b09fa984701489a247d9770 Mon Sep 17 00:00:00 2001 From: christopher-besch Date: Mon, 28 Apr 2025 06:37:08 +0000 Subject: [PATCH] better comments and variable names for ActionRunNowDone (#7697) Document a bit more clearly what the difference between the parameters to `sendActionRunNowDoneNotificationIfNeeded` and `ActionRunNowDone` are: `priorState`, `run`, `lastRun`, `updatedRun`. The new variable names should be more explicit and consistent. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [x] I do not want this change to show in the release notes. - [ ] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/7697 Reviewed-by: Earl Warren Co-authored-by: christopher-besch Co-committed-by: christopher-besch --- services/actions/notifier.go | 27 ++++++++++++++++----------- services/notify/notify.go | 2 ++ 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/services/actions/notifier.go b/services/actions/notifier.go index 756738608b..f1e9a6d7e9 100644 --- a/services/actions/notifier.go +++ b/services/actions/notifier.go @@ -781,9 +781,14 @@ func (n *actionsNotifier) MigrateRepository(ctx context.Context, doer, u *user_m }).Notify(ctx) } -func sendActionRunNowDoneNotificationIfNeeded(ctx context.Context, oldRun, newRun *actions_model.ActionRun) error { - if !oldRun.Status.IsDone() && newRun.Status.IsDone() { - lastRun, err := actions_model.GetRunBefore(ctx, newRun.RepoID, newRun.Stopped) +// Call this sendActionRunNowDoneNotificationIfNeeded when there has been an update for an ActionRun. +// priorRun and updatedRun represent the very same ActionRun, just at different times: +// priorRun before the update and updatedRun after. +// The parameter lastRun in the ActionRunNowDone notification represents an entirely different ActionRun: +// the ActionRun of the same workflow that finished before priorRun/updatedRun. +func sendActionRunNowDoneNotificationIfNeeded(ctx context.Context, priorRun, updatedRun *actions_model.ActionRun) error { + if !priorRun.Status.IsDone() && updatedRun.Status.IsDone() { + lastRun, err := actions_model.GetRunBefore(ctx, updatedRun.RepoID, updatedRun.Stopped) if err != nil && !errors.Is(err, util.ErrNotExist) { return err } @@ -793,10 +798,10 @@ func sendActionRunNowDoneNotificationIfNeeded(ctx context.Context, oldRun, newRu return err } } - if err = newRun.LoadAttributes(ctx); err != nil { + if err = updatedRun.LoadAttributes(ctx); err != nil { return err } - notify_service.ActionRunNowDone(ctx, newRun, oldRun.Status, lastRun) + notify_service.ActionRunNowDone(ctx, updatedRun, priorRun.Status, lastRun) } return nil } @@ -804,7 +809,7 @@ func sendActionRunNowDoneNotificationIfNeeded(ctx context.Context, oldRun, newRu // wrapper of UpdateRunWithoutNotification with a call to the ActionRunNowDone notification channel func UpdateRun(ctx context.Context, run *actions_model.ActionRun, cols ...string) error { // run.ID is the only thing that must be given - oldRun, err := actions_model.GetRunByID(ctx, run.ID) + priorRun, err := actions_model.GetRunByID(ctx, run.ID) if err != nil { return err } @@ -813,11 +818,11 @@ func UpdateRun(ctx context.Context, run *actions_model.ActionRun, cols ...string return err } - newRun, err := actions_model.GetRunByID(ctx, run.ID) + updatedRun, err := actions_model.GetRunByID(ctx, run.ID) if err != nil { return err } - return sendActionRunNowDoneNotificationIfNeeded(ctx, oldRun, newRun) + return sendActionRunNowDoneNotificationIfNeeded(ctx, priorRun, updatedRun) } // wrapper of UpdateRunJobWithoutNotification with a call to the ActionRunNowDone notification channel @@ -832,7 +837,7 @@ func UpdateRunJob(ctx context.Context, job *actions_model.ActionRunJob, cond bui } runID = oldJob.RunID } - oldRun, err := actions_model.GetRunByID(ctx, runID) + priorRun, err := actions_model.GetRunByID(ctx, runID) if err != nil { return 0, err } @@ -842,9 +847,9 @@ func UpdateRunJob(ctx context.Context, job *actions_model.ActionRunJob, cond bui return affected, err } - newRun, err := actions_model.GetRunByID(ctx, runID) + updatedRun, err := actions_model.GetRunByID(ctx, runID) if err != nil { return affected, err } - return affected, sendActionRunNowDoneNotificationIfNeeded(ctx, oldRun, newRun) + return affected, sendActionRunNowDoneNotificationIfNeeded(ctx, priorRun, updatedRun) } diff --git a/services/notify/notify.go b/services/notify/notify.go index 9c50f69059..02c18272cb 100644 --- a/services/notify/notify.go +++ b/services/notify/notify.go @@ -377,6 +377,8 @@ func ChangeDefaultBranch(ctx context.Context, repo *repo_model.Repository) { } // ActionRunNowDone notifies that the old status priorStatus with (priorStatus.isDone() == false) of an ActionRun changed to run.Status with (run.Status.isDone() == true) +// run represents the new state of the ActionRun. +// lastRun represents the ActionRun of the same workflow that finished before run. // lastRun might be nil (e.g. when the run is the first for this workflow). It is the last run of the same workflow for the same repo. // It can be used to figure out if a successful run follows a failed one. // Both run and lastRun need their attributes loaded.