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.

Sign up for GitHub

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

Jump to bottom

Add Gitea Webhook #1755

Merged
merged 3 commits into from
May 29, 2017
Merged

Add Gitea Webhook #1755

merged 3 commits into from
May 29, 2017

Conversation

DblK
Copy link
Member

@DblK DblK commented May 19, 2017

While checking the webhook code, I saw that there were some gogs information.
So I fix the typo and the webhook test.

go2sh
go2sh reviewed May 19, 2017
View reviewed changes
models/webhook.go Outdated
@@ -1,4 +1,4 @@
// Copyright 2014 The Gogs Authors. All rights reserved.
// Copyright 2017 Gitea. All rights reserved.
Copy link

@go2sh go2sh May 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, you can add a line. But the copyright to the old stuff cannot be removed, FMPOV.

Copy link
Member Author

Choose a reason for hiding this comment

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

For other files, the copyright has been changed, so I did the same way but I do not care
I will wait for the review to be sure I should keep it or not.

Copy link
Member

Choose a reason for hiding this comment

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

Only Gitea copyright is in files that are new and was not in Gogs. In places where new things are added new copyright line is added, see:

gitea/main.go

Line 2 in a201977

// Copyright 2016 The Gitea Authors. All rights reserved.

Also it should be The Gitea Authors

Copy link
Member

Choose a reason for hiding this comment

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

You should add the copyright above the Gogs-line when changing something :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I took the example from this file https://github.com/go-gitea/gitea/blob/master/models/status.go#L1

@lafriks
Copy link
Member

lafriks commented May 19, 2017

These changes will break backwards compatibility with existing integrations and API

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 19, 2017
@bkcsoft
Copy link
Member

bkcsoft commented May 19, 2017

My biggest issue is that the Drone-integration will break, so I'd wait for harness/harness#1978 first :)

@bkcsoft
Copy link
Member

bkcsoft commented May 19, 2017

And we need a migration to fix all the current webhooks 🙄

@DblK
Copy link
Member Author

DblK commented May 19, 2017 •
edited
Loading

There is a PR waiting for integration: harness/harness#2017.
I agree that we should do it after the drone integration.

Good point for the migration. I won't do it right now because webhook might change. But once the drone PR is merged, I will work on it (if I have free time :p)

@lunny lunny added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label May 19, 2017
@lunny lunny added this to the 1.3.0 milestone May 19, 2017
@appleboy appleboy mentioned this pull request May 24, 2017
Merged
@DblK
Copy link
Member Author

DblK commented May 24, 2017

Since harness/harness#2017 has been merged, how we should finish this PR?

@lunny
Copy link
Member

lunny commented May 24, 2017

@DblK, like @lafriks said, add a new webhook type Gitea is a good chosen but not change Gogs to Gitea so that we also have the ability to migrate Gogs' webhooks to Gitea when user migrate their's Gogs to Gitea.

@lunny lunny removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label May 24, 2017
@DblK DblK changed the title Replace Gogs by Gitea in Webhook Add Gitea Webhook May 24, 2017
@lunny
Copy link
Member

lunny commented May 25, 2017

Looks good.

@DblK DblK mentioned this pull request May 26, 2017
Closed
@strk
Copy link
Member

strk commented May 28, 2017

LGTM

@DblK
Copy link
Member Author

DblK commented May 28, 2017

Make L-G-T-M work!

@tboerger tboerger 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 May 28, 2017
@appleboy
Copy link
Member

LGTM

@appleboy
Copy link
Member

make L-G-T-M work

@tboerger tboerger 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 May 29, 2017
@appleboy appleboy merged commit e0c6ab2 into go-gitea:master May 29, 2017
@lunny lunny modified the milestones: 1.2.0, 1.3.0 May 29, 2017
@lunny
Copy link
Member

lunny commented May 29, 2017

@appleboy Please check the milestone before merge the PR.

@appleboy
Copy link
Member

@lunny ok. my fault. I forgot to check the milestone.

@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Aug 25, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@go2sh go2sh go2sh left review comments

@lafriks lafriks lafriks left review comments

@bkcsoft bkcsoft bkcsoft left review comments

@appleboy appleboy appleboy approved these changes

Assignees
No one assigned
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
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.
8 participants
@DblK @lafriks @bkcsoft @lunny @strk @appleboy @go2sh @tboerger