Status-API #1332

bkcsoft · 2017-03-19T21:41:57Z

Initial support for Status-API

Closes #357

TODO:

lunny · 2017-03-20T01:01:36Z

models/migrations/v24.go

+	"github.com/go-xorm/xorm"
+)
+
+type StatusTable struct {


Maybe CommitStatus is better than StatusTable?

lunny · 2017-03-20T01:04:10Z

models/migrations/v24.go

+	Index       int64  `xorm:"INDEX UNIQUE(repo_sha_index)"`
+	RepoID      int64  `xorm:"INDEX UNIQUE(repo_sha_index)"`
+	State       string `xorm:"TEXT NOT NULL"`
+	SHA         string `xorm:"TEXT NOT NULL INDEX UNIQUE(repo_sha_index)"`


don't use TEXT since SHA has a fixed length.

Since there's talk about replacing SHA1 in git with "something else" we probably shouldn't make assumptions about it's length.

But some databases will be failed if the index column is too long like mysql and mariadb. This happend on #1283 .

lunny · 2017-03-20T01:04:48Z

models/migrations/v24.go

+
+type StatusTable struct {
+	ID          int64  `xorm:"pk autoincr"`
+	Index       int64  `xorm:"INDEX UNIQUE(repo_sha_index)"`


what's the Index meaning?

It is the Counter across Repo and SHA. Each SHA has up to 1000 statuses (I don't actually limit, github api docs specifies it though).

lunny · 2017-03-20T01:07:12Z

models/status.go

+	"github.com/go-xorm/xorm"
+)
+
+type StatusState string


lunny · 2017-03-20T01:07:59Z

models/status.go

+	StatusWarning StatusState = "warning"
+)
+
+type Status struct {


maybe CommitStatus is better?

bkcsoft · 2017-03-20T01:58:01Z

@lunny All things should be fixed 🙂

bkcsoft · 2017-03-20T05:04:11Z

Well, fully working 🎉

lunny · 2017-03-20T05:26:07Z

build failed and conflicted

go2sh · 2017-03-20T08:16:13Z

models/status.go

+	// CommitStatusFailure is for when the Status is Failure
+	CommitStatusFailure CommitStatusState = "failure"
+	// CommitStatusWarning is for when the Status is Warning
+	CommitStatusWarning CommitStatusState = "warning"


Hm, I never saw that in any other hosting software. From my point of view, this state is not necessary. Warnings are part of the build processes, never the less its still successful.

It's an idea that I got then I have flaky tests/internet. Basically if a test fails I wanna restart it once automatically, if it success on second try mark with 'warning' 🙂 (good for when you're getting rate-limited by Alpines Repo CDN 😂 )

In any case, it's a super-set of GitHub API, so it won't hurt to have it 🙂

Fair point, wasn't sure about the use case. ;-) I never got in touch which such problems since all our company builds run locally without any external dependencies. On the other hand, having such problems in your tests is also not ideal.^^

Well, got the initial idea when I had intermittant race-conditions in my tests. Re-running the tests made 'em pass, but you should obviously fix the race-condition, hence "warning" 😉

The reason I want it thought is mainly for when you're rate-limited. I want it to return "warning" since you should fix the issue, but it might not be critical to fix it now 🙂

sapk · 2017-03-20T10:20:48Z

You need to have a fixed length for string index (like https://github.com/go-gitea/gitea/pull/1332/files#diff-cbd97f0ad3f0f48c8a314bbcfaf60f0cR19) to be compatible with mysql/mariadb (http://stackoverflow.com/questions/1827063/mysql-error-key-specification-without-a-key-length). If git change the format we only have to upgrade it size or create a sub-object.

Trust me, I learned it the hard way ^^

appleboy · 2017-03-29T03:11:15Z

conflicts

strk · 2017-04-06T17:47:14Z

Go @bkcsoft go ! :)
I'm looking forward to see better CI integration ... #996

appleboy · 2017-04-07T04:42:34Z

Yeah I'm looking forward to see better CI integration too. 👍 Go !!!!

bkcsoft · 2017-04-07T08:31:43Z

@lunny please re-review :)

appleboy · 2017-04-07T08:33:55Z

models/status_test.go

+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+// +build disabled


Why need build tag?

Because the tests were failing 😂 Thanks for pointing that out

lunny · 2017-04-07T13:16:26Z

@bkcsoft build failed. the new table affected the tests.

bkcsoft · 2017-04-10T20:27:53Z

@lunny now it should build ;)

strk · 2017-04-10T20:29:15Z

But it still conflicts @bkcsoft

bkcsoft · 2017-04-10T20:47:04Z

Rebased, resolved conflicts, squashed, builds and tests passing, ready to go! 🎉

bkcsoft · 2017-04-10T21:22:29Z

Done!

appleboy · 2017-04-11T01:28:40Z

LGTM

lunny · 2017-04-11T02:42:48Z

models/status.go

+		sess.Rollback()
+		return fmt.Errorf("newCommitStatus[%s, %s]: %v", opts.Repo.RepoPath(), opts.SHA, err)
+	}
+	sess.Commit()


return sess.Commit()

lunny · 2017-04-11T04:28:20Z

models/migrations/v28.go

+	ID          int64  `xorm:"pk autoincr"`
+	Index       int64  `xorm:"INDEX UNIQUE(repo_sha_index)"`
+	RepoID      int64  `xorm:"INDEX UNIQUE(repo_sha_index)"`
+	State       string `xorm:"TEXT NOT NULL"`


State should also have a fixed size.

lunny · 2017-04-11T05:55:56Z

models/migrations/v28.go

@@ -0,0 +1,30 @@
+package migrations


comment head

lunny · 2017-04-19T05:21:31Z

@bkcsoft conflicts.

lunny · 2017-04-19T14:27:28Z

@bkcsoft still conflicted

bkcsoft · 2017-04-21T07:36:27Z

@lunny Done! 🎉 Merge It! 😂

strk · 2017-04-21T08:56:25Z

LGTM go go go ! Looking forward to Drone reporting build status of PRs ! :)

bkcsoft · 2017-04-21T15:26:06Z

@strk Me too, but I'm not gonna be the one doing that unfortunately. I really don't have any time to spare ATM 😢

bkcsoft

@lunny Here's the breakage that I can find :)

bkcsoft · 2017-04-21T15:34:19Z

models/status.go

+	defer sess.Close()
+	if err = sess.Begin(); err != nil {
+		return fmt.Errorf("newCommitStatus[%s, %s]: %v", opts.Repo.RepoPath(), opts.SHA, err)
+	}


This sess & Begin isn't required

JohnTheodore · 2017-05-09T22:44:24Z

Can I generate an oauth2 token with limited scope to update the commit status ? (like github or bitbucket does). eg, I could give this token to drone to allow seamless security controlled access

bkcsoft · 2017-05-10T09:25:00Z

@JohnTheodore You can generate a Deploy Token for use with this. In your Repository goto Settings then Deploy Keys and press Add Deploy Key 🙂 (We really need to work on documentation 😂 )

jerrykan · 2017-11-14T03:01:16Z

This seems to have been merged but I don't see any reference to it in swagger and I can't find any other documentation about it. Should the Status API stuff show up in swagger?

jerrykan · 2017-11-14T03:31:25Z

Don't worry. I finally tracked down the new swagger location on try.gitea.io and it looks like a lot of things have been fixed since the v1.2.x release. I look forward to 1.3.x.

bkcsoft added modifies/api This PR adds API routes or modifies them type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Mar 19, 2017

bkcsoft added this to the 1.2.0 milestone Mar 19, 2017

bkcsoft force-pushed the bkcsoft/status-api branch from 7cfc844 to ee11e3a Compare March 20, 2017 00:38

lunny requested changes Mar 20, 2017

View reviewed changes

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 20, 2017

bkcsoft changed the title ~~WIP: Status-API~~ Status-API Mar 20, 2017

bkcsoft added the pr/wip This PR is not ready for review label Mar 20, 2017

bkcsoft force-pushed the bkcsoft/status-api branch from d16d6cd to 7ba765a Compare March 20, 2017 07:35

go2sh reviewed Mar 20, 2017

View reviewed changes

lunny mentioned this pull request Mar 29, 2017

Gitea hosted Gitea #1029

Open

20 tasks

bkcsoft force-pushed the bkcsoft/status-api branch from 7ba765a to ee1f2c0 Compare April 7, 2017 08:19

appleboy reviewed Apr 7, 2017

View reviewed changes

bkcsoft force-pushed the bkcsoft/status-api branch from 9c1a7ce to 5f81701 Compare April 10, 2017 20:46

appleboy approved these changes Apr 11, 2017

View reviewed changes

tboerger removed the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 11, 2017

tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 11, 2017

lunny requested changes Apr 11, 2017

View reviewed changes

lunny removed the pr/wip This PR is not ready for review label Apr 11, 2017

lunny reviewed Apr 11, 2017

View reviewed changes

models/migrations/v28.go Outdated

@@ -0,0 +1,30 @@

package migrations

Copy link

Member

lunny Apr 11, 2017

Choose a reason for hiding this comment

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

comment head

ekozan mentioned this pull request Apr 14, 2017

Gitea (community-driven Gogs) integration harness/harness#1978

Closed

bkcsoft force-pushed the bkcsoft/status-api branch from 0b8ad58 to 503015a Compare April 19, 2017 13:10

Status-API

27990d5

bkcsoft force-pushed the bkcsoft/status-api branch from 503015a to 27990d5 Compare April 21, 2017 07:35

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 Apr 21, 2017

lunny approved these changes Apr 21, 2017

View reviewed changes

lunny merged commit 4bea219 into master Apr 21, 2017

lunny mentioned this pull request Apr 21, 2017

Something is causing a lock on some records on "repositories" table #1529

Closed

7 tasks

bkcsoft commented Apr 21, 2017

View reviewed changes

lunny mentioned this pull request Apr 22, 2017

fix bug on status API #1533

Merged

appleboy mentioned this pull request May 1, 2017

feat: add gitea remote driver. harness/harness#2017

Merged

bkcsoft deleted the bkcsoft/status-api branch May 10, 2017 09:26

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Status-API #1332

Status-API #1332

Conversation

bkcsoft commented Mar 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft commented Mar 20, 2017

bkcsoft commented Mar 20, 2017

lunny commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

go2sh Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapk commented Mar 20, 2017 • edited Loading

appleboy commented Mar 29, 2017

strk commented Apr 6, 2017

appleboy commented Apr 7, 2017

bkcsoft commented Apr 7, 2017

Choose a reason for hiding this comment

bkcsoft Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

lunny commented Apr 7, 2017

bkcsoft commented Apr 10, 2017

strk commented Apr 10, 2017

bkcsoft commented Apr 10, 2017 • edited Loading

bkcsoft commented Apr 10, 2017

appleboy commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Apr 19, 2017

lunny commented Apr 19, 2017

bkcsoft commented Apr 21, 2017

strk commented Apr 21, 2017

bkcsoft commented Apr 21, 2017

bkcsoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnTheodore commented May 9, 2017

bkcsoft commented May 10, 2017

jerrykan commented Nov 14, 2017

jerrykan commented Nov 14, 2017

bkcsoft commented Mar 19, 2017 •

edited

Loading

go2sh Mar 20, 2017 •

edited

Loading

sapk commented Mar 20, 2017 •

edited

Loading

bkcsoft Apr 10, 2017 •

edited

Loading

bkcsoft commented Apr 10, 2017 •

edited

Loading