Record RSL entry for all pushes #7
Conversation
|
This needs testing still. |
adityasaky
force-pushed
the
record-all-pushes
branch
4 times, most recently
from
817c3a3 to
3c83611
Compare
|
There's one issue still with this. We see a pull request event and a push event when a PR is merged. For the former, we record a PR attestation and then an RSL entry for the base branch where the merge happened. With the latter, we just record an RSL entry: the intent is to model "regular pushes" to a feature branch rather than a push as a result of a PR merge. What's a straightforward way of identifying in the push event that it's actually a PR merge and not recording the RSL entry? One option may be to hit the API for the merge commit and see if it's associated with any PRs and then inspect those to see if they were merged? But that feels expensive? @wlynch thoughts? |
internal/webhook/webhook.go
Outdated
| return err | ||
| } | ||
|
|
||
| cloneURL := *event.Repo.CloneURL |
There was a problem hiding this comment.
nit: the safer thing to do for these fields accesses (here and elsewhere) is use the getters so that we don't panic if any of the fields aren't set.
There was a problem hiding this comment.
good shout, I'll update that.
internal/webhook/webhook.go
Outdated
| return err | ||
| } | ||
|
|
||
| if err := repo.RecordRSLEntryForReference("refs/gittuf/local-ref", true, rslopts.WithOverrideRefName(*event.Ref)); err != nil { |
There was a problem hiding this comment.
What's the significance of local-ref here?
There was a problem hiding this comment.
This was to be able to fetch the ref that was pushed to without having to handle cases such as the pushed branch is already checked out (default branch). It was either do this or check whether event.Ref matches the repo's default branch / currently checked out branch and skip fetch in that instance.
FWIW, we're not cloning with event.Ref as the to-be-cloned ref to also support tags cleanly, though I may have an incorrect assumption there.
I believe merged PRs you should be able to detect with their state/event type (closed). It is possible to get associated PRs from a push commit using the graphql API, but you also need to be able to handle the possibility that a single commit can be associated with multiple PRs in various states of it being merged. edit: you don't even need the graphql api: https://docs.github.com/en/rest/commits/commits?apiVersion=2022-11-28#list-pull-requests-associated-with-a-commit. I think if you blanket ignore closed PRs though (whether that's from a merge or a manual close) you'd still get the behavior you want and save yourself an API call. |
Correct, for the pull request event. However, a pull request merge also triggers a push event and we want to handle all push events except those that are a result of a pull request merge. |
adityasaky
force-pushed
the
record-all-pushes
branch
2 times, most recently
from
0f2b364 to
cbac8b4
Compare
Signed-off-by: Aditya Sirish A Yelgundhalli <ayelgundhall@bloomberg.net>
Signed-off-by: Aditya Sirish A Yelgundhalli <ayelgundhall@bloomberg.net>
adityasaky
force-pushed
the
record-all-pushes
branch
from
cbac8b4 to
3482573
Compare
|
@wlynch I've added an API call to fetch PRs for the commit now. This check skips recording an entry if a PR is merged and the base branch matches the push branch. |
|
I'm going to merge this to deploy this to the public app (and test). I'll file a ticket to continue discussing if we can avoid the extra API call. |
Fixes #6