Monitor PRs on repository, and add branch information on Postgres #25
Conversation
The pipeline now cares about individual PRs and stores PR information in the database.
| @@ -3,7 +3,8 @@ GRANT ALL PRIVILEGES ON DATABASE docker TO docker; | |||
| CREATE TABLE benchmarks( | |||
| repositories varchar(256), | |||
| commits varchar(50) NOT NULL UNIQUE, | |||
| json_data jsonb | |||
| json_data jsonb, | |||
| branch varchar(256) | |||
There was a problem hiding this comment.
| branch varchar(256) | |
| branch varchar(256) |
| @@ -12,7 +13,8 @@ CREATE TABLE benchmarksrun ( | |||
| time float8, | |||
| ops_per_sec float8, | |||
| mbs_per_sec float8, | |||
| timestamp float8 | |||
| timestamp float8, | |||
| branch varchar(256) | |||
There was a problem hiding this comment.
I think we don't need branch in both tables. The commit can be used to retrieve which branch is used from the benchmarks table.
| ^ ") " | ||
|
|
||
| let get_repo json = Yojson.Basic.from_string json |> member "repo" |> to_string | ||
|
|
||
| let get_data_from_json commit json = | ||
| let get_data_from_json commit json pr = |
There was a problem hiding this comment.
it's a bit confusing to have the function get_data_from_json add the pr string, as it is not extracted from the json. I would add pr to the result separately.
| @@ -74,32 +77,37 @@ let get_data_from_json commit json = | |||
| (metrics |> member "time" |> to_float |> string_of_float) | |||
| (metrics |> member "mbs_per_sec" |> to_float |> string_of_float) | |||
| (metrics |> member "ops_per_sec" |> to_float |> string_of_float)) | |||
| (string_of_float (Unix.time ()))) | |||
| (string_of_float (Unix.time ())) | |||
There was a problem hiding this comment.
| (string_of_float (Unix.time ())) | |
| Unix.time () |> string_of_float |
so that it's similar to the lines above
| @@ -39,7 +39,8 @@ let read_file path = | |||
|
|
|||
| open Yojson.Basic.Util | |||
|
|
|||
| let format_benchmark_data commit name time mbs_per_sec ops_per_sec timestamp = | |||
| let format_benchmark_data commit name time mbs_per_sec ops_per_sec timestamp pr | |||
There was a problem hiding this comment.
can you rename name into bench_name ? I confused it with the repo name, for which sometimes you use the variable name.
| let pipeline ~github ~(repo : Github.Repo_id.t) ?slack_path ?docker_cpu | ||
| ?docker_numa_node ~docker_shm_size ~conninfo () = | ||
| let name = repo.name in | ||
| let repo = Current.return (github, repo) in |
There was a problem hiding this comment.
it's a bit weird that you construct a term Current.t from a pair which then you deconstruct in https://github.com/gs0510/index-benchmarks/pull/25/files#diff-bb69bf8bfba5fa2eb0e602d7891536ecR33.
Why not pass both arguments to the function process_pipeline ?
I don't understand current well enough, maybe you need to construct this pair to ensure re-evaluation?
| let+ refs = | ||
| Current.component "Get PRs" | ||
| |> let> api, repo = repo in | ||
| Github.Api.refs api repo |
There was a problem hiding this comment.
Maybe use ci_refs ? It would make this code easier to read.
There was a problem hiding this comment.
We can;t get PR numbers or any branch information in ci_refs hence using refs.
There was a problem hiding this comment.
Ah, I see, you only get the commits with ci_refs.
| |> let** path, _ = p in | ||
| let channel = read_channel_uri path in | ||
| Slack.post channel ~key:"output" (Current.map snd p)) | ||
| |> Current.ignore_value) |
There was a problem hiding this comment.
you can get rid of this line if you replace https://github.com/gs0510/index-benchmarks/pull/25/files#diff-bb69bf8bfba5fa2eb0e602d7891536ecR41 with Current.return None
| let name = repo.name in | ||
| let repo = Current.return (github, repo) in | ||
| process_pipeline ~docker_cpu ~docker_numa_node ~docker_shm_size ~conninfo ~name ~repo | ||
| ~slack_path | ||
| |> Current.ignore_value |
There was a problem hiding this comment.
you can also remove this line, by ensuring that process_pipeline return unit Current.t. I think it should be feasible as Slack.post returns unit Current.t and that's the last function you call.
| @@ -39,7 +39,8 @@ let read_file path = | |||
|
|
|||
| open Yojson.Basic.Util | |||
|
|
|||
There was a problem hiding this comment.
What do you think of adding utils.mli? You can keep better track of unused functions and also add some documentation.
No description provided.