Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Support workflow event dispatch via API #33545

Merged
merged 16 commits into from
Feb 10, 2025

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Feb 10, 2025

Fix: #31765 (Re-open #32059)


Major changes after #32059

  1. remove WorkflowAPI interface because these route handlers are not reused, it's not like the other one besides it.
  2. use golang err handling approach (wrap&as) to process errors with locale
  3. remove unnecessary workflowID != "" check because the path param can't be empty if the handler has been routed to.
  4. fix path & query escaping
  5. fix error handling (NotExisting -> 404)
  6. use "map[string]string" for "inputs" (before it used "any" which seems too relax and might cause problems)
  7. decouple web Context (it should never manually construct any web/api Context)
  8. update comments and reformat some tests

Co-authored-by: Bence Santha git@santha.eu
Co-authored-by: Christopher Homberger christopher.homberger@web.de

ref: go-gitea#31765

---------

Signed-off-by: Bence Santha <git@santha.eu>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: Christopher Homberger <christopher.homberger@web.de>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 10, 2025
@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 10, 2025
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Feb 10, 2025
Copy link
Contributor

@ChristopherHX ChristopherHX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree your Locale Wrap approach looks better

Copy link
Contributor

@ChristopherHX ChristopherHX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have added the nil check in the callback to have the opportunity to later reject inputs not defined for the workflow, but since this is not implemented removing this is ok

@wxiaoguang
Copy link
Contributor Author

I think I have added the nil check in the callback to have the opportunity to later reject inputs not defined for the workflow, but since this is not implemented removing this is ok

Hmm, I thought that we could reject the inputs in the outer caller function, then no need to check nil in every "processInputs" callback. Does it seem good?

@wxiaoguang wxiaoguang marked this pull request as draft February 10, 2025 15:31
@ChristopherHX
Copy link
Contributor

I thought that we could reject the inputs in the outer caller function
Does it seem good?

Yes we can do it like this, if we set all values into the map provided to the function. Just had different thoughts in my mind this is fine.

Another open detail I left behind has been case insensitivity of inputs. Technically is should be ok to pass INPUT to an input with name Input. (Not relevant for the UI)

@ChristopherHX
Copy link
Contributor

ChristopherHX commented Feb 10, 2025

I just tested github.com, they are ignoring content-type: application/x-www-form-urlencoded and don't like formdata.

Gitea ignores json if this header is found

gh cli also converts form to json on the fly before making the call

@wxiaoguang wxiaoguang marked this pull request as ready for review February 10, 2025 16:17
@wxiaoguang wxiaoguang force-pushed the support-workflow-dispatch-api branch from c41a613 to 54ffed2 Compare February 10, 2025 16:23
Copy link
Contributor

@ChristopherHX ChristopherHX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 10, 2025
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 10, 2025
@lunny lunny added this to the 1.24.0 milestone Feb 10, 2025
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 10, 2025
@wxiaoguang wxiaoguang merged commit 30993e9 into go-gitea:main Feb 10, 2025
26 checks passed
@wxiaoguang wxiaoguang deleted the support-workflow-dispatch-api branch February 10, 2025 19:05
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 10, 2025
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 11, 2025
* giteaofficial/main:
  Enhance routers for the Actions runner operations (go-gitea#33549)
  [skip ci] Updated translations via Crowdin
  Run yamllint with strict mode, fix issue (go-gitea#33551)
  Enhance routers for the Actions variable operations (go-gitea#33547)
  enhancement: add additional command hints for PowerShell & CMD (go-gitea#33548)
  Feature: Support workflow event dispatch via API (go-gitea#33545)
  Optimize the dashboard (go-gitea#32990)
  Rework suggestion backend (go-gitea#33538)
  Revert "Feature: Support workflow event dispatch via API (go-gitea#32059)" (go-gitea#33541)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I suggests adding a new endpoint to facilitate the dispatching of workflows.
5 participants