Alternative fix for #229, random temp ID #231
Conversation
| @@ -242,6 +244,7 @@ func run(c *cli.Context) error { | |||
| }, | |||
| Build: docker.Build{ | |||
| Remote: c.String("remote.url"), | |||
| Ref: c.String("commit.sha"), | |||
There was a problem hiding this comment.
the ref should be set to c.String("commit.ref")
There was a problem hiding this comment.
The goal here was to not pollute the org.label-schema.vcs-ref label with the random ID, so I copied "Name" which was the sha hash. If the term is confusing, maybe it should be called CommitSha?
There was a problem hiding this comment.
the ref is not a random string, it is a git ref, for example refs/heads/master. I think the overall change is fine, but the ref should use the actual ref value.
There was a problem hiding this comment.
That's true, however it would break existing behavior as this label contained the commit sha before the change by using the Name value.
There was a problem hiding this comment.
I see, org.label-schema.vcs-ref is supposed to be the commit sha. I recommend removing this and sending a separate pull request. This requires additional discussion. We can merge the random Name in the short term.
There was a problem hiding this comment.
@techknowlogick that would be awesome! Last I checked Gitea did not send this information in the payload. See https://github.com/drone/go-scm/blob/master/scm/driver/gitea/testdata/webhooks/tag_create.json
There was a problem hiding this comment.
Just tested and there is a key in the webhook response labled: sha. Let me send a PR to go-scm to add this information.
There was a problem hiding this comment.
PR is here: drone/go-scm#22
There was a problem hiding this comment.
Thank you for a great fix there @techknowlogick.
I had a dig to see if the same would apply to gogs, but unfortunately it seems there's no sha here.
https://github.com/gogs/go-gogs-client/blob/5898069c37e1044229e3853adba654d61e45816b/repo_hook.go#L114
Example tag payload
{
"ref": "v2.4.3",
"ref_type": "tag",
"default_branch": "master",
"repository": {
"id": 1,
"owner": {
"id": 1,
"username": "example",
"login": "example",
"full_name": "",
"email": "[email protected]",
"avatar_url": "https://git.example.com/avatars/1"
},
"name": "hello-cicd",
"full_name": "example/hello-cicd",
"description": "",
"private": true,
"fork": false,
"parent": null,
"empty": false,
"mirror": false,
"size": 1300480,
"html_url": "https://git.example.com/example/hello-cicd",
"ssh_url": "ssh://[email protected]/example/hello-cicd.git",
"clone_url": "https://git.example.com/example/hello-cicd.git",
"website": "",
"stars_count": 0,
"forks_count": 0,
"watchers_count": 1,
"open_issues_count": 0,
"default_branch": "master",
"created_at": "2018-04-17T20:45:00Z",
"updated_at": "2019-04-19T09:20:56Z"
},
"sender": {
"id": 1,
"username": "example",
"login": "example",
"full_name": "",
"email": "[email protected]",
"avatar_url": "https://git.example.com/avatars/1"
}
}vs gittea's counterpart here
https://github.com/go-gitea/go-sdk/blob/master/gitea/hook.go#L196
There was a problem hiding this comment.
I've opened the PR there, we'll have to see what they feel about this change.
gogs/gogs#5689
| if plugin.Build.Name == "" { | ||
| // Use hex encoding as docker requires lowercase. | ||
| // Note: we can't use 64 characters as it conflicts with sha256 image hashes. | ||
| plugin.Build.Name = uniuri.NewLenChars(40, hexChars) |
There was a problem hiding this comment.
40 chars? Why do you want to define such a long random name?
There was a problem hiding this comment.
40 matches the keyspace of git's sha1 so it has similar properties in terms of collisions.
| @@ -242,6 +244,7 @@ func run(c *cli.Context) error { | |||
| }, | |||
| Build: docker.Build{ | |||
| Remote: c.String("remote.url"), | |||
| Ref: c.String("commit.sha"), | |||
There was a problem hiding this comment.
Just add an SHA attribute and rename Ref to it. That's not so much confusing. Beside this change from Name to Ref/SHA you should not change the label schema, we still got to discuss how to continue with that since label-schema.org is deprecated.
Closes #229