adds datetime tagger tagpolicy #621
Conversation
balopat
requested review from
dgageot,
dlorenc,
r2d4 and
viglesiasce
as code owners
pkg/skaffold/util/clock.go
Outdated
| limitations under the License. | ||
| */ | ||
|
|
||
| package util |
There was a problem hiding this comment.
I think this can just live in date_time.go and the tagger package, not sure what else we could use it for?
There was a problem hiding this comment.
yeah, that's where I had it the first time - but then I figured if we'd need time/date related mocking anywhere else, this could serve as a pattern, save some "time". Happy to move it back and do the extraction to util when the second use case actually occurs!
pkg/skaffold/util/clock.go
Outdated
| type RealClock struct{} | ||
|
|
||
| func (RealClock) Now() time.Time { | ||
| return time.Now() |
There was a problem hiding this comment.
Or you could just get rid of this. I don't think we'd have other implementations of the interface or other functions on it, so i think it would be safe to just make it
type dateTimeTagger struct {
Format string
TimeZone string
timeFn func()time.Time
}
tagger := dateTimeTagger{timeFn: time.Now}
mockTagger := dateTimeTagger{timeFn: func()time.Time { return test.time } }Might save a bit of code, but either way SGTM
There was a problem hiding this comment.
👍
nice! I guess similarly to above, we can introduce the Clock interface later if it's needed (when there are multiple functions there to be mocked).
pkg/skaffold/build/tag/date_time.go
Outdated
| "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" | ||
| ) | ||
|
|
||
| const tagTime = "2006-01-02_15-04-05.999_MST" |
There was a problem hiding this comment.
is this a valid image tag? I don't think we do any validation in the code on whether or not the tags we produce are valid according to the image tag spec
There was a problem hiding this comment.
yeah, this is valid. here's a snippet from a skaffold output for the default format:
...
Build complete in 13.545233439s
gcr.io/***/svc-issues -> gcr.io/***/svc-issues:2018-05-31_11-05-38.609_PDT
gcr.io/***/petclinic-skaffold -> gcr.io/***/petclinic-skaffold:2018-05-31_11-05-38.609_PDT
...
I was thinking about validation, but then the tool fails with a very clear message when tagging fails, for example for
dateTime:
format: "2006-01-02_15:04:05"
The colons are not valid, so:
Successfully tagged 345165a2228c4b2f255f5eabcdd5724e:latest
FATA[0000] build step: tagging image: Error parsing reference: "gcr.io/***/svc-issues:2018-05-31_18:03:00" is not a valid repository/tag: invalid reference format
There was a problem hiding this comment.
I think as long as the default format is valid its fine
pkg/skaffold/build/tag/date_time.go
Outdated
| return "", fmt.Errorf("bad timezone provided: \"%s\", error: %s", timezone, err) | ||
| } | ||
|
|
||
| return opts.ImageName + ":" + c.Now().In(loc).Format(format), nil |
| format = tagger.Format | ||
| } | ||
|
|
||
| timezone := "Local" |
There was a problem hiding this comment.
nit: i'd move this down to right before time.LoadLocation, I had to look this up to figure out what it did :)
There was a problem hiding this comment.
hmmm...it is right before it - is there a more concise way in go to assign "Local as the default value if tagger.TimeZone is not defined"?
|
yay! |
|
@balopat can you add example usage? |
|
@tuananh I'll add an issue for it. I did add the section in annotated-skaffold.yaml - I hope that helps in the meantime. |
Fixes #309.