diff options
| -rw-r--r-- | mod/mod-ci-github-gq.cxx | 21 | ||||
| -rw-r--r-- | mod/mod-ci-github-gq.hxx | 16 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.hxx | 2 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 64 |
4 files changed, 44 insertions, 59 deletions
diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx index 17096eb..1909e1f 100644 --- a/mod/mod-ci-github-gq.cxx +++ b/mod/mod-ci-github-gq.cxx @@ -241,7 +241,8 @@ namespace brep { vector<gh_check_run> check_runs; // Received check runs. - resp (json::parser& p): check_runs (gq_parse_response_check_runs (p)) {} + resp (json::parser& p) + : check_runs (gq_parse_response_check_runs (p)) {} resp () = default; } rs; @@ -271,23 +272,23 @@ namespace brep // @@ Are we handling the case where the resulting state (built) // differs from what we expect? // - if (rst != build_state::built && rst != st) + // @@@ Does built-to-built transition updates status? + // + if (rst != st && rst != build_state::built) { error << "unexpected check_run status: received '" << rcr.status << "' but expected '" << gh_to_status (st) << '\''; return false; // Fail because something is clearly very wrong. } - else - { - check_run& cr (crs[i]); - if (!cr.node_id) - cr.node_id = move (rcr.node_id); + check_run& cr (crs[i]); - cr.state = rst; - cr.state_synced = true; - } + if (!cr.node_id) + cr.node_id = move (rcr.node_id); + + cr.state = rst; + cr.state_synced = (rst == st); } return true; diff --git a/mod/mod-ci-github-gq.hxx b/mod/mod-ci-github-gq.hxx index 3083696..9022fe3 100644 --- a/mod/mod-ci-github-gq.hxx +++ b/mod/mod-ci-github-gq.hxx @@ -20,8 +20,8 @@ namespace brep // // Create a new check run on GitHub for each build. Update `check_runs` with - // the new states and node IDs. Return false and issue diagnostics if the - // request failed. + // the new data (node id, state, and state_synced). Return false and issue + // diagnostics if the request failed. // // Note: no details_url yet since there will be no entry in the build result // search page until the task starts building. @@ -40,8 +40,8 @@ namespace brep build_state); // Create a new check run on GitHub for a build. Update `cr` with the new - // state and the node ID. Return false and issue diagnostics if the request - // failed. + // data (node id, state, and state_synced). Return false and issue + // diagnostics if the request failed. // // If the details_url is absent GitHub will use the app's homepage. // @@ -69,8 +69,12 @@ namespace brep // Update a check run on GitHub. // // Send a GraphQL request that updates an existing check run. Update `cr` - // with the new state. Return false and issue diagnostics if the request - // failed. + // with the new data (state and state_synced). Return false and issue + // diagnostics if the request failed. + // + // Note that GitHub allows any state transitions except from built (but + // built to built is allowed). The latter case is signalled by setting the + // check_run state_synced member to false and the state member to built. // // If the details_url is absent GitHub will use the app's homepage. // diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx index eb81ae4..0dd52ca 100644 --- a/mod/mod-ci-github-service-data.hxx +++ b/mod/mod-ci-github-service-data.hxx @@ -33,7 +33,7 @@ namespace brep build_state state; bool state_synced; - optional<result_status> status; // Only present if state is built. + optional<result_status> status; // Only if state is built & synced. string state_string () const diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index e7dd7b5..b2e0c41 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -1736,34 +1736,8 @@ namespace brep if (cr.state_synced) { - // Check run was created/updated successfully to built. - // - // @@ TMP Feels like this should also be done inside - // gq_{create,update}_check_run() -- where cr.state is set if the - // create/update succeeds -- but I think we didn't want to pass a - // result_status into a gq_ function because converting to a GitHub - // conclusion/title/summary is reasonably complicated. - // - // @@@ We need to redo that code: - // - // - Pass the vector of check runs with new state (and status) set. - // - Update synchronized flag inside those functions. - // - Update the state to built if it's already built on GitHub -- - // but then what do we set the status to? - // - // @@ TMP This scenario can only arise for updates to building. - // For creations a new CR will always be created so the - // returned state will always be what we asked for; and we - // never update to queued. - // - // As for updates to building, if GH has already been updated - // to built then the build_built() lambda will soon save the - // built state and valid status so nothing more should need to - // be done. In fact, whenever updating to building we do stop - // immediately if it's already built on GH. - // - // - Maybe signal in return value (optional<bool>?) that there is - // a discrepancy. + // Check run was created/updated successfully to built (with + // status we specified). // cr.status = b.status; @@ -1833,24 +1807,30 @@ namespace brep if (iat) sd.installation_access = *iat; - if (check_run* scr = sd.find_check_run (cr.build_id)) + // Only update the check_run state in service data if it matches the + // state (specifically, status) on GitHub. + // + if (cr.state_synced) { - // This will most commonly generate a duplicate warning (see above). - // We could save the old state and only warn if it differs but let's - // not complicate things for now. - // -#if 0 - if (scr->state != build_state::building) + if (check_run* scr = sd.find_check_run (cr.build_id)) { - warn << "check run " << cr.build_id << ": out of order built " - << "notification service data update; existing state: " - << scr->state_string (); - } + // This will most commonly generate a duplicate warning (see above). + // We could save the old state and only warn if it differs but let's + // not complicate things for now. + // +#if 0 + if (scr->state != build_state::building) + { + warn << "check run " << cr.build_id << ": out of order built " + << "notification service data update; existing state: " + << scr->state_string (); + } #endif - *scr = cr; + *scr = cr; + } + else + sd.check_runs.push_back (cr); } - else - sd.check_runs.push_back (cr); return sd.json (); }; |