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

Add Matrix webhook #10831

Merged
merged 14 commits into from
Mar 28, 2020
Merged

Add Matrix webhook #10831

merged 14 commits into from
Mar 28, 2020

Conversation

S7evinK
Copy link
Contributor

@S7evinK S7evinK commented Mar 26, 2020

This PR adds the possibility to add Matrix webhooks as requested in #6590.
Since the the URL displayed is a little bit long (spanning multiple lines), we might need to change

<a class="dont-break-out" href="{{$.BaseLink}}/{{.ID}}">{{.URL}}</a>
to contain a trimmed version of the URL to make it look better, or is there a better way to only display up to x chars of the URL?

Signed-off-by: Till Faelligen <tfaelligen@gmail.com>
Signed-off-by: Till Faelligen <tfaelligen@gmail.com>
Signed-off-by: Till Faelligen <tfaelligen@gmail.com>
Signed-off-by: Till Faelligen <tfaelligen@gmail.com>
@lafriks lafriks added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Mar 26, 2020
@lafriks lafriks added this to the 1.12.0 milestone Mar 26, 2020
Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

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

Did not test but looks good ;)

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 26, 2020
@S7evinK
Copy link
Contributor Author

S7evinK commented Mar 26, 2020

I did some tests myself:
image

And like mentioned, the Webhooks overview doesn't look that nice with very long URLs:
image

modules/webhook/matrix.go Outdated Show resolved Hide resolved
modules/webhook/matrix.go Outdated Show resolved Hide resolved
Signed-off-by: Till Faelligen <tfaelligen@gmail.com>
Signed-off-by: Till Faelligen <tfaelligen@gmail.com>
@codecov-io
Copy link

codecov-io commented Mar 27, 2020

Codecov Report

Merging #10831 into master will decrease coverage by 0.09%.
The diff coverage is 20.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10831      +/-   ##
==========================================
- Coverage   43.49%   43.40%   -0.10%     
==========================================
  Files         592      593       +1     
  Lines       82979    83265     +286     
==========================================
+ Hits        36094    36141      +47     
- Misses      42399    42632     +233     
- Partials     4486     4492       +6     
Impacted Files Coverage Δ
models/webhook.go 65.97% <0.00%> (-0.35%) ⬇️
modules/auth/repo_form.go 43.69% <0.00%> (-0.75%) ⬇️
modules/webhook/deliver.go 45.16% <0.00%> (-1.25%) ⬇️
modules/webhook/webhook.go 42.36% <0.00%> (-1.53%) ⬇️
routers/repo/webhook.go 1.11% <0.00%> (-0.12%) ⬇️
modules/webhook/matrix.go 28.88% <28.88%> (ø)
modules/setting/webhook.go 56.25% <100.00%> (ø)
routers/routes/routes.go 86.47% <100.00%> (+0.09%) ⬆️
modules/indexer/stats/db.go 40.62% <0.00%> (-18.75%) ⬇️
modules/indexer/stats/queue.go 62.50% <0.00%> (-18.75%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cd4704...b0c473e. Read the comment docs.

@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 Mar 27, 2020
Copy link
Contributor

@tulir tulir left a comment

Choose a reason for hiding this comment

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

Matrix events accept arbitrary JSON, so it might be nice to send some raw webhook data too. That way it could be used as a full alternative for traditional webhooks instead of just a chat thing. The only limit is that events can't be more than ~64 KiB, not sure how likely webhook data is to hit that.

Something like this:

{
  "msgtype": "m.text",
  "body": "...",
  "io.gitea.webhook": {
    "commits": [...]
  }
}

routers/routes/routes.go Outdated Show resolved Hide resolved
templates/repo/settings/webhook/new.tmpl Outdated Show resolved Hide resolved
routers/repo/webhook.go Outdated Show resolved Hide resolved
Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

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

As per comments above

@S7evinK
Copy link
Contributor Author

S7evinK commented Mar 28, 2020

Using the Authorization header feels like a dirty workaround. Couldn't figure out a better solution.

@lafriks lafriks merged commit 828a27f into go-gitea:master Mar 28, 2020
@OahzEgroeg
Copy link

Can't add matrix as default webhooks or system webooks from site administration, when select matrix, it doesn't return any form.

@techknowlogick
Copy link
Member

@EgroegOahz please create a ticket for this.

@go-gitea go-gitea locked and limited conversation to collaborators Jun 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants