forgejo/forgejo

Fork 696

WIP: Add a PeekTask endpoint to the actions API #5849

Closed

wetneb wants to merge 1 commit from wetneb/forgejo:5847-PeekTask-API into forgejo

wetneb commented

2024-11-07 15:29:12 +01:00

Member

This lets a runner check if a task is available for it, without getting the task assigned to it immediately.

For #5847. So far just an untested proof of concept.

Problems

This area of the code doesn't seem covered by tests much so it's unclear to me how to set them up. If people have pointers to tests to take inspiration from, I'd be very grateful.

Also, this new PeekTask API is reusing the protobuf request/responses from the FetchTask endpoint. Is that fine? Intuitively, the response at least ought to be separate, because we shouldn't return a task, but rather a boolean indicating if a task is available, or even better, the number of pending tasks (so that one can scale a pool of runners dynamically), as suggested by @dcz_pf. But it seems to me that the protobuf declarations are not part of this code base, so it would be harder to modify them as things stand. Alternatively, we could make separate Protobuf declarations in the forgejo repository only for this API endpoint, without forking the others.

Also, I couldn't figure out how to specify that PeekTask expects GET requests, compared to FetchTask which should be called with POST requests only (I think?)

Checklist

Tests

I added test coverage for Go changes...
- in their respective *_test.go for unit tests.
- in the tests/integration directory if it involves interactions with a live Forgejo server.
I added test coverage for JavaScript changes...
- in web_src/js/*.test.js if it can be unit tested.
- in tests/e2e/*.test.e2e.js if it requires interactions with a live Forgejo server (see also the developer guide for JavaScript testing).

Documentation

I created a pull request to the documentation to explain to Forgejo users how to use this change.
I did not document these changes and I do not expect someone else to do it.

Release notes

I do not want this change to show in the release notes.
I want the title to show in the release notes with a link to this pull request.
I want the content of the release-notes/<pull request number>.md to be be used for the release notes instead of the title.

This lets a runner check if a task is available for it, without getting the task assigned to it immediately. For #5847. So far just an untested proof of concept. ## Problems This area of the code doesn't seem covered by tests much so it's unclear to me how to set them up. If people have pointers to tests to take inspiration from, I'd be very grateful. Also, this new `PeekTask` API is reusing the protobuf request/responses from the `FetchTask` endpoint. Is that fine? Intuitively, the response at least ought to be separate, because we shouldn't return a task, but rather a boolean indicating if a task is available, or even better, the number of pending tasks (so that one can scale a pool of runners dynamically), as [suggested](https://codeberg.org/dcz_pf/ci_fetcher) by @dcz_pf. But it seems to me that the protobuf declarations are not part of this code base, so it would be harder to modify them as things stand. Alternatively, we could make separate Protobuf declarations in the forgejo repository only for this API endpoint, without forking the others. Also, I couldn't figure out how to specify that `PeekTask` expects GET requests, compared to `FetchTask` which should be called with POST requests only (I think?) ## Checklist ### Tests - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [ ] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [ ] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title.

❤️ 1

wetneb added the

test

needed

label

2024-11-07 15:29:12 +01:00

Gusted requested review from algernon

2024-11-08 00:34:44 +01:00

earl-warren commented

2024-11-14 08:17:20 +01:00

Contributor

Please rebase your pull request or merge the latest target branch. It is necessary for the tests to not be skipped. A change in how the CI is run was done 12 hours ago and the requirement it imposes on open pull requests was overlooked. Sorry for the inconvenience.

wetneb force-pushed 5847-PeekTask-API from 4f6cebaecc

/ backporting (pull_request_target) Has been skipped

Details

testing / frontend-checks (pull_request) Successful in 1m32s

Details

/ info (pull_request_target) Successful in 3s

Details

/ cascade (pull_request_target) Has been skipped

Details

testing / backend-checks (pull_request) Successful in 2m59s

Details

testing / test-e2e (pull_request) Successful in 2m2s

Details

testing / test-unit (pull_request) Successful in 5m14s

Details

testing / test-remote-cacher (map[image:docker.io/bitnami/redis:7.2 port:6379]) (pull_request) Successful in 1m39s

Details

testing / test-remote-cacher (map[image:docker.io/bitnami/valkey:7.2 port:6379]) (pull_request) Successful in 1m56s

Details

testing / test-remote-cacher (map[image:ghcr.io/microsoft/garnet-alpine:1.0.14 port:6379]) (pull_request) Successful in 1m55s

Details

testing / test-remote-cacher (map[image:registry.redict.io/redict:7.3.0-scratch port:6379]) (pull_request) Successful in 1m49s

Details

testing / test-mysql (pull_request) Successful in 22m1s

Details

testing / test-sqlite (pull_request) Successful in 22m16s

Details

/ release-notes (pull_request_target) Has been skipped

Details

testing / test-pgsql (pull_request) Successful in 27m7s

Details

testing / security-check (pull_request) Successful in 1m22s

Details

to 9c99755dcf

/ release-notes (pull_request_target) Has been skipped

Details

testing / frontend-checks (pull_request) Successful in 1m10s

Details

testing / backend-checks (pull_request) Successful in 2m35s

Details

testing / test-e2e (pull_request) Successful in 2m25s

Details

testing / test-unit (pull_request) Successful in 4m28s

Details

testing / test-remote-cacher (map[image:docker.io/bitnami/redis:7.2 port:6379]) (pull_request) Successful in 1m52s

Details

testing / test-remote-cacher (map[image:docker.io/bitnami/valkey:7.2 port:6379]) (pull_request) Successful in 2m48s

Details

testing / test-remote-cacher (map[image:ghcr.io/microsoft/garnet-alpine:1.0.14 port:6379]) (pull_request) Successful in 2m25s

Details

testing / test-remote-cacher (map[image:registry.redict.io/redict:7.3.0-scratch port:6379]) (pull_request) Successful in 2m19s

Details

testing / test-mysql (pull_request) Successful in 21m34s

Details

testing / test-sqlite (pull_request) Successful in 21m59s

Details

testing / test-pgsql (pull_request) Successful in 24m32s

Details

testing / security-check (pull_request) Successful in 55s

Details

requirements / merge-conditions (pull_request) Failing after 2s

Details

issue-labels / cascade (pull_request_target) Has been skipped

Details

issue-labels / release-notes (pull_request_target) Has been skipped

Details

issue-labels / backporting (pull_request_target) Has been skipped

Details

milestone / set (pull_request_target) Has been skipped

Details

2024-11-14 08:19:06 +01:00

Compare

wetneb referenced this pull request from forgejo/discussions

2024-11-21 20:39:36 +01:00

Proposal: Enhancing Forgejo Actions with Autoscaling Capabilities #241

earl-warren added the

valuable code

label

2025-02-02 12:36:28 +01:00

earl-warren commented

2025-02-02 12:36:39 +01:00

Contributor

This pull request contains good code but it has not seen any activity in a long time. In order to unclutter the queue and help reviewers focus on those that are actively worked on, it is now closed. Feel free to re-open it as soon as you want. It is also marked with the label valuable code so it can conveniently be found later and not confused with pull requests closed for other reasons.

This pull request contains good code but it has not seen any activity in a long time. In order to unclutter the queue and help reviewers focus on those that are actively worked on, it is now closed. Feel free to re-open it as soon as you want. It is also marked with the label `valuable code` so it can conveniently be found later and not confused with pull requests closed for other reasons.

earl-warren closed this pull request

2025-02-02 12:36:44 +01:00