Support tracking state for multiple repos #97

yasunariw · 2020-12-23T12:12:10Z

Description of the task

Separates config + pipeline status tracking by repository, so that the bot can send notifications for multiple repositories, each according to its own config file.

Changes:

Track separate config + state information for each repo
Give users the option to configure GitHub secrets per repo (see documentation/config_docs.md for details). ~~Doesn't break existing deployments as this per-repo config is optional.~~ This is now mandatory Support tracking state for multiple repos #97 (comment)
However, existing state.json files will be incompatible with new format

How to test

Existing tests should pass (make test). Also tested with staging workspace.

References

existing issue: Support multiple gh repos #41

yasunariw · 2020-12-31T02:15:24Z

~~Postponing this PR until remote config loading behavior is changed to fetch on startup~~

yasunariw · 2022-01-07T02:58:38Z

lib/state.ml

-let empty : State_t.state = { pipeline_statuses = StringMap.empty; bot_user_id = None }
+type t = {
+  state : State_t.state;
+  lock : Lwt_mutex.t;


lock because pipeline_statuses is a mutable field containing a string map (I think this was necessary even before multi-repo)

this must be a comment in code

yasunariw · 2022-01-07T02:59:38Z

lib/state.ml

@@ -2,19 +2,45 @@ open Base
 open Common
 open Devkit

-let empty : State_t.state = { pipeline_statuses = StringMap.empty; bot_user_id = None }
+type t = {


later should add mli and make opaque

yasunariw · 2022-01-07T03:02:07Z

lib/context.ml

@@ -85,16 +95,17 @@ let refresh_state ctx =
      match get_local_file path with
      | Error e -> fmt_error "error while getting local file: %s\nfailed to get state from file %s" e path
      | Ok file ->
-        let state = State_j.state_of_string file in
+        let state = { ctx.state with state = State_j.state_of_string file } in


later should move some of this fn into state.ml

maybe leave that as a comment in code

Khady · 2022-01-10T00:34:18Z

documentation/secret_docs.md

+## `allowed_repos`
+
+Use this option to restrict incoming notifications from GitHub to approved repository URLs.
+
+Repository URLs should be fully qualified (include the protocol), with no trailing backslash.
+


I don't understand why this second value is required. Aren't all the whitelisted repos already listed under repos?

This was to 1) stay compatible with existing configs, 2) let repos be whitelisted without defining repo-specifc secrets, and 3) let whitelisting itself be optional. I could alternatively use repos as the whitelist (and expect null for whitelisted repos that use default secret), or rename to something less confusing like repo_secrets

I think we should break compatibility in such cases, or support two formats - legacy and new one (have one object per repository with secrets and any other configurations necessary per-repo)

@Khady @rr0gi Hopefully after 1.5 years the stars will align for me to finally merge this.

I broke compatibility, now there is a single repos field. If empty then assumes single repo. Also I changed from object to list, to allow for repos that don't need to override any secrets.

233543c

With repo-specific secrets, we'll need the repo name in order to obtain the webhook token used for signature validation. So parsing the request body for a GH payload needs to happen before, not after, the signature check.

Define custom getters for retrieving GH secret values. As the getter for each token type defaults to looking for a global value if a repo-specific vaue isn't found, existing deployments don't need to change.

For each test case, initializes a repo state from file if one exists.

yasunariw force-pushed the yasu/upgrade-status-rules branch from b47ab24 to 24e0c4f Compare December 24, 2020 02:19

yasunariw force-pushed the yasu/support-multiple-repos branch from 0dce9e2 to 9112794 Compare December 24, 2020 03:16

yasunariw changed the base branch from yasu/upgrade-status-rules to master December 24, 2020 03:18

yasunariw force-pushed the yasu/support-multiple-repos branch 3 times, most recently from d983684 to 64c4582 Compare December 25, 2020 13:01

yasunariw mentioned this pull request Dec 25, 2020

Add tests for multi-repo setup #100

Open

yasunariw marked this pull request as ready for review December 25, 2020 13:09

yasunariw requested a review from ygrek December 25, 2020 13:09

yasunariw mentioned this pull request Dec 29, 2020

Modularize dependencies for GH payload consumption and Slack message sending #92

Merged

yasunariw force-pushed the yasu/support-multiple-repos branch from 64c4582 to bdaa5e8 Compare December 29, 2020 01:57

yasunariw removed the request for review from ygrek December 31, 2020 02:12

yasunariw force-pushed the yasu/support-multiple-repos branch 3 times, most recently from 6741fb6 to d306035 Compare January 5, 2021 06:11

yasunariw requested review from Khady and ygrek January 5, 2021 06:29

yasunariw force-pushed the master branch from 76a7905 to 00852d8 Compare November 30, 2021 06:28

yasunariw force-pushed the yasu/support-multiple-repos branch 4 times, most recently from d3fe839 to 3e47dde Compare December 3, 2021 10:29

yasunariw force-pushed the yasu/support-multiple-repos branch from ec155eb to 1792647 Compare January 7, 2022 02:30

yasunariw commented Jan 7, 2022

View reviewed changes

Khady reviewed Jan 10, 2022

View reviewed changes

yasunariw removed the request for review from ygrek May 5, 2022 02:57

yasunariw added 3 commits May 6, 2022 16:20

add atd wrapper for hash tables

c38c36c

cfg: make ctx store a hash tbl mapping urls to cfgs

8003841

state: store runtime repo state in a hash table indexed by repo url

5bd7727

yasunariw added 9 commits May 6, 2022 16:20

ctx: allocate new hash table for every Context.make ()

cc9c3f7

github: move webhook signature checking logic out of parse_exn

3774234

With repo-specific secrets, we'll need the repo name in order to obtain the webhook token used for signature validation. So parsing the request body for a GH payload needs to happen before, not after, the signature check.

cfg: allow repo-specific configuration of gh_token and gh_hook_token

4ad34de

Define custom getters for retrieving GH secret values. As the getter for each token type defaults to looking for a global value if a repo-specific vaue isn't found, existing deployments don't need to change.

cfg: add option to restrict handling of GH payloads to certain repo urls

19a1a09

test: recreate context per test case

c12c551

For each test case, initializes a repo state from file if one exists.

docs: add info on multi-repo setup

ce425ba

state: add lock

7a3f6fb

add comments

6813222

cfg: unify repos and allowed_repos into singlee field

233543c

yasunariw force-pushed the yasu/support-multiple-repos branch from 9697bed to 233543c Compare May 6, 2022 10:04

yasunariw requested review from Khady and rr0gi May 6, 2022 10:14

yasunariw added 2 commits May 7, 2022 02:04

fmt

32a2ce2

cfg: make specifying repos mandatory

49a0150

yasunariw merged commit 1eab7b5 into master Jun 28, 2022

yasunariw deleted the yasu/support-multiple-repos branch June 28, 2022 07:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support tracking state for multiple repos #97

Support tracking state for multiple repos #97

yasunariw commented Dec 23, 2020 •

edited

Loading

yasunariw commented Dec 31, 2020 •

edited

Loading

yasunariw Jan 7, 2022

Khady Jan 10, 2022

yasunariw Jan 7, 2022

yasunariw Jan 7, 2022

Khady Jan 10, 2022

Khady Jan 10, 2022

yasunariw Jan 10, 2022

rr0gi Jan 11, 2022

yasunariw May 6, 2022

Support tracking state for multiple repos #97

Support tracking state for multiple repos #97

Conversation

yasunariw commented Dec 23, 2020 • edited Loading

Description of the task

How to test

References

yasunariw commented Dec 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasunariw commented Dec 23, 2020 •

edited

Loading

yasunariw commented Dec 31, 2020 •

edited

Loading