Avoid job duplication by Github webhooks #34
Conversation
tallysmartins
force-pushed
the
webhooks_improvements
branch
from
5de6bef
to
a322256
Compare
| @@ -63,6 +73,18 @@ defmodule ElixirBench.Benchmarks do | |||
| Repo.all(Job) | |||
| end | |||
|
|
|||
| def get_or_create_job(repo, attrs) do | |||
There was a problem hiding this comment.
I like introducing this abstraction! 👍
There was a problem hiding this comment.
Speaking of abstraction I like - I think this deserves unit tests. Like the 2 major cases, but also what happens if we get 2 commits with the same sha but from different repositories (see my argument above)
There was a problem hiding this comment.
yeah, I added test cases to cover this... in the future if we care about the branch name and not just the repo and commit sha the tests will cover this scenario and fail
| @@ -51,6 +51,16 @@ defmodule ElixirBench.Benchmarks do | |||
| Repo.fetch(where(Job, uuid: ^uuid)) | |||
| end | |||
|
|
|||
| def fetch_running_job_by_reference(branch_name, commit_sha) do | |||
There was a problem hiding this comment.
do we need this to be public atm?
| Job | ||
| |> last | ||
| |> Job.unfinished() | ||
| |> where(branch_name: ^branch_name, commit_sha: ^commit_sha) |
There was a problem hiding this comment.
where does the reference come from? I wonder if it's an appropriate name for it.
I also think we can just drop the branch name - commit shas shouldn't duplicate so we're save there. This seems to be missing any scoping by repository or so which we should rather introduce imo
There was a problem hiding this comment.
reference I meant branch name and commit... but you are right, this is not the best name. I will also add a check for the repo as I said in my previous comment.
About skipping the branch name, I agree that it's possible and simpler. But I like the idea of separate builds for each branch to maintain the history and not just respond with the previous results.. But then we think about evolving this feature.
There was a problem hiding this comment.
added the scope and changed to a more sigificative name
| query = | ||
| Job | ||
| |> last | ||
| |> Job.unfinished() |
There was a problem hiding this comment.
why do we scope by unfinished? If it's the same sha for the same repo, I don't care if it's still running or not I don't think we should run it again unless a user explicitly requests this and we don't have this feature yet :)
There was a problem hiding this comment.
Yeah, in fact knowing if there is an existent job for a branch and commit should be enough, regardless if its running or finished. And I missed the repo part here.. it must be checked.
tallysmartins
force-pushed
the
webhooks_improvements
branch
4 times, most recently
from
bfde612
to
8257b78
Compare
| |> Job.unfinished() | ||
| |> where(branch_name: ^branch_name, commit_sha: ^commit_sha) | ||
| |> Job.filter_by_repo(repo.id) | ||
| |> where(commit_sha: ^commit_sha) |
There was a problem hiding this comment.
as we removed last won't this return multiple jobs now instead of just one? I think the code still works but it's confusing and we potentially do more than we should
There was a problem hiding this comment.
you are right 💯... Repo.fetch will break if more than one Job is found... I changed a test case for that and put back the last
There was a problem hiding this comment.
had to do a little change in tests as we just check jobs for a given repo and commit-sha, ignoring the branch name... so a test from WebhookController was failing
There was a problem hiding this comment.
Repo.fetch wouldn't fail - Repo.one would fail :)
There was a problem hiding this comment.
Repo.fetch calls one in the end, see ElixirBench.Repo, I tested and it breaks 🕵️♂️
tallysmartins
force-pushed
the
webhooks_improvements
branch
3 times, most recently
from
5c6dd9f
to
94c8ffe
Compare
- closes elixir-bench#29 Signed-off-by: Tallys Martins <tallysmartins@gmail.com>
Signed-off-by: Tallys Martins <tallysmartins@gmail.com>
tallysmartins
force-pushed
the
webhooks_improvements
branch
from
94c8ffe
to
e7d0a84
Compare
Signed-off-by: Tallys Martins tallysmartins@gmail.com