-
Notifications
You must be signed in to change notification settings - Fork 323
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
Alternative fix for #229, random temp ID #231
Open
Beanow
wants to merge
1
commit into
drone-plugins:master
Choose a base branch
from
Beanow:pr/fallback-random-tag
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ package main | |
import ( | ||
"os" | ||
|
||
"github.com/dchest/uniuri" | ||
"github.com/joho/godotenv" | ||
"github.com/sirupsen/logrus" | ||
"github.com/urfave/cli" | ||
|
@@ -11,7 +12,8 @@ import ( | |
) | ||
|
||
var ( | ||
version = "unknown" | ||
version = "unknown" | ||
hexChars = []byte("abcdef0123456789") | ||
) | ||
|
||
func main() { | ||
|
@@ -242,6 +244,7 @@ func run(c *cli.Context) error { | |
}, | ||
Build: docker.Build{ | ||
Remote: c.String("remote.url"), | ||
Ref: c.String("commit.sha"), | ||
Name: c.String("commit.sha"), | ||
Dockerfile: c.String("dockerfile"), | ||
Context: c.String("context"), | ||
|
@@ -290,5 +293,13 @@ func run(c *cli.Context) error { | |
} | ||
} | ||
|
||
// Make sure we always have a name when commit.sha is not available. | ||
// See https://github.com/drone-plugins/drone-docker/issues/229 | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 40 chars? Why do you want to define such a long random name? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 40 matches the keyspace of git's sha1 so it has similar properties in terms of collisions. |
||
} | ||
|
||
return plugin.Exec() | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the ref should be set to
c.String("commit.ref")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 calledCommitSha
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is here: drone/go-scm#22
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened the PR there, we'll have to see what they feel about this change.
gogs/gogs#5689