web-platform-tests / wpt.fyi

web-platform-tests dashboard
https://wpt.fyi/
Other
182 stars 88 forks source link

Fix #3906: Implement a /api/checks/ endpoint for GitHub Actions #3970

Closed gsnedders closed 1 week ago

gsnedders commented 2 weeks ago

This is very much a derivative of the implementation used for Azure Pipelines, perhaps unsurprising given the similarity between the two systems, but is very much different enough to justify a totally separate implementation.

Like Azure Pipelines, we rely on a notification from the workflow that it is complete, rather than relying on webhooks from GitHub (as Taskcluster does), as this allows the workflow to provide metadata to endpoint, rather than having to hard code so many magic strings in wpt.fyi (notably, the name of the artifacts we're looking for).

I'm not really sure how best to test this, and am welcome to feedback here.

Fixes #3906.

gsnedders commented 2 weeks ago

It's currently failing within the lint, with:

api/ghactions/notify.go:175:1: chooseLabels returns interface (github.com/deckarep/golang-set.Set) (ireturn)

Given that package only has interfaces in its public API, I'm not really sure what the idiomatic way of doing this. Have a pointer to the interface and then dereference it everywhere it is used?

gsnedders commented 1 week ago

For testing, I see that the azure package doesn't have more tests than this, but maybe you could use taskcluster as inspiration? I'm thinking that perhaps something like this would be useful.

(I assume that's meant to point at api/taskcluster/webhook_test.go but either way…)

Given the relatively large number of calls into the GitHub API I'm somewhat lost with gomock (really I'm lost in general with Go!), but maybe if someone else has a suggestion…?

past commented 1 week ago

For testing, I see that the azure package doesn't have more tests than this, but maybe you could use taskcluster as inspiration? I'm thinking that perhaps something like this would be useful.

(I assume that's meant to point at api/taskcluster/webhook_test.go but either way…)

Oops, I fat-fingered pasting the link there. I did mean this as you rightly surmised.

Given the relatively large number of calls into the GitHub API I'm somewhat lost with gomock (really I'm lost in general with Go!), but maybe if someone else has a suggestion…?

Perhaps the checks API tests are an even better fit here, then. You would likely need to refactor notify.go to use an api.go that can be mocked, similarly to these other examples. I realize that this is not a trivial amount of work, so I'd be OK with filing an issue to handle this in a follow-up.

past commented 1 week ago

I looked over the new changes and this still LGTM.

gsnedders commented 1 week ago

@past this should be deployed, and a github-actions user added (with appropriate credentials in the secrets data store) so that it can all work. https://github.com/gsnedders/web-platform-tests/actions/runs/10730968742/job/29760852391 has it trying to hit the endpoint this adds.

past commented 1 week ago

Secrets created, you should be able to hit the endpoint in staging already and in prod once #3983 is closed.