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

Teamcity webhook #18668

Closed
wants to merge 31 commits into from
Closed

Teamcity webhook #18668

wants to merge 31 commits into from

Conversation

firephreek
Copy link

Implements a basic TeamCity Webhook.

TC Webhooks require:

  • HostUrl
  • Auth Token
  • VCS Root Id/Name

TC Hook is just a REST API that takes a POST with no body and an "Authorization: Bearer " Header.

Translations properties added, templates created, logo included.

Go isn't my primary or even secondary language, so happy to hear changes that would be needed.
Adding the BearerToken property to the HookTask seemed the least problematic approach to getting the AuthToken included in the final POST, but if there's a more 'GO' approach or something specific to the project, I don't know it.

Add enum/strings referencing TeamCity
…t have a body for posts.

Add teamcity to webhook types
Add handling for TeamCity Hook Task.

TeamCity needs an 'Authorization: Bearer <token>' header to work. Added a new property to models\HookTask and a check for value. If the value is there, the header is added. Otherwise, it's omitted.
@lunny
Copy link
Member

lunny commented Feb 8, 2022

Hi, you have an outdate code base.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 8, 2022
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

Haven't taken a look at the code, but there are a few nitpicks that the CI will likely fail on.

Thanks for the PR :)

modules/webhook/teamcity.go Outdated Show resolved Hide resolved
modules/webhook/teamcity.go Outdated Show resolved Hide resolved
options/locale/locale_fa-IR.ini Outdated Show resolved Hide resolved
templates/swagger/v1_json.tmpl Outdated Show resolved Hide resolved
Add enum/strings referencing TeamCity
…t have a body for posts.

Add teamcity to webhook types
Add handling for TeamCity Hook Task.

TeamCity needs an 'Authorization: Bearer <token>' header to work. Added a new property to models\HookTask and a check for value. If the value is there, the header is added. Otherwise, it's omitted.
…webhook

# Conflicts:
#	models/webhook.go
#	modules/auth/repo_form.go
#	modules/setting/webhook.go
#	modules/webhook/deliver.go
#	modules/webhook/webhook.go
#	options/locale/locale_cs-CZ.ini
#	options/locale/locale_de-DE.ini
#	options/locale/locale_en-US.ini
#	options/locale/locale_es-ES.ini
#	options/locale/locale_fa-IR.ini
#	options/locale/locale_fi-FI.ini
#	options/locale/locale_fr-FR.ini
#	options/locale/locale_hu-HU.ini
#	options/locale/locale_it-IT.ini
#	options/locale/locale_ja-JP.ini
#	options/locale/locale_ko-KR.ini
#	options/locale/locale_lv-LV.ini
#	options/locale/locale_nl-NL.ini
#	options/locale/locale_pl-PL.ini
#	options/locale/locale_pt-BR.ini
#	options/locale/locale_pt-PT.ini
#	options/locale/locale_ru-RU.ini
#	options/locale/locale_sr-SP.ini
#	options/locale/locale_sv-SE.ini
#	options/locale/locale_tr-TR.ini
#	options/locale/locale_uk-UA.ini
#	options/locale/locale_zh-CN.ini
#	options/locale/locale_zh-TW.ini
#	routers/repo/webhook.go
#	routers/routes/routes.go
#	templates/admin/hook_new.tmpl
#	templates/org/settings/hook_new.tmpl
#	templates/repo/settings/webhook/list.tmpl
#	templates/repo/settings/webhook/new.tmpl
This reverts commit 6c68fa57ece29258f0e5cdf0a0b962d0d2f34265.
Add Authorization Header when Bearer Token is present during Webhook delivery.
Add empty TeamCity Payload builder to accommodate future expansion and stay consistent with existing patterns.
@firephreek
Copy link
Author

PR Rebased. Thought I was at latest but upstream wasn't pulling correctly, should be resolved.
Added a small unit test for the webhook, just checks that the payload isn't altered and that the Meta content is marshalled correctly.

@firephreek firephreek mentioned this pull request Feb 9, 2022
@KN4CK3R
Copy link
Member

KN4CK3R commented Feb 9, 2022

This contains changes which should not be part of this PR.

@firephreek
Copy link
Author

This contains changes which should not be part of this PR.

Found and reverted the offending files. I think they got picked up when I rebased.

{{else if eq .HookType "msteams"}}
<img width="26" height="26" src="{{AssetUrlPrefix}}/img/msteams.png">
{{else if eq .HookType "teamcity"}}
<img width="26" height="26" src="{{AssetUrlPrefix}}/img/teamcity.png">
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate image?

{{else if eq .HookType "msteams"}}
<img width="26" height="26" src="{{AssetUrlPrefix}}/img/msteams.png">
{{else if eq .HookType "teamcity"}}
<img width="26" height="26" src="{{AssetUrlPrefix}}/img/teamcity.png">
Copy link
Member

Choose a reason for hiding this comment

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

duplicate?

{{template "repo/settings/webhook/msteams" .}}
{{template "repo/settings/webhook/teamcity" .}}
Copy link
Member

Choose a reason for hiding this comment

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

duplicate?

Copy link
Author

Choose a reason for hiding this comment

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

I hate rebasing so much sometimes....resolved! Thanks for the catch!

@zeripath
Copy link
Contributor

zeripath commented Feb 9, 2022

This branch is a total mess. You've reverted lots of important things and caused loads of conflicts.

Do you have a SHA from before you attempted to rebase?

Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

There are lots of serious reversions here due to mismerges. This cannot merge as is.

@LecrisUT
Copy link
Contributor

@firephreek don't be afraid to cherrypick/rebase and force push on your own branch to clean up the commit history.

@devInteam
Copy link

any news on this issue?

@wxiaoguang
Copy link
Contributor

any news on this issue?

This PR's merge history is still broken.

And in future I think all webhooks could be covered by a general mechanism, instead of hard-code more and more webhooks.

@techknowlogick
Copy link
Member

closing per above comment

@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.