bake: add unixtimestampparse function#3286
Conversation
e70b98b to
7a36d1f
Compare
|
One uses $ SOURCE_DATE_EPOCH=0 docker buildx bake
$ SOURCE_DATE_EPOCH=42 docker buildx bakeDoes this PR support this use case?
— Having this |
This is already supported as a standardized environment variable Edit: We only have docs for it in https://docs.docker.com/build/ci/github-actions/reproducible-builds/ but I think we should also have some for build and bake. Maybe in https://docs.docker.com/build/building/variables/#build-tool-configuration-variables. cc @ArthurFlag |
|
Maybe I was not clear enough in #3197: $ SOURCE_DATE_EPOCH=0 docker buildx bakelabels = {
"org.opencontainers.image.created" = "how do I get SOURCE_DATE_EPOCH as an RFC3339-formatted string here"
}The label should look like |
33d0ecb to
ec0bb73
Compare
ec0bb73 to
8df6fc5
Compare
Pushed extra commit to add |
856f62b to
e4913a3
Compare
docs/bake-stdlib.md
Outdated
| * `year_day` (Number) The day of the year for the timestamp, in the range 1-365 for non-leap years, and 1-366 in leap years. | ||
| * `day` (Number) The day of the month for the timestamp. | ||
| * `month` (Number) The month of the year for the timestamp. | ||
| * `month_name` (String) The name of the month for the timestamp (ex. "January"). |
There was a problem hiding this comment.
How does this work in different locales?
january/Januar/janvier/Январь/มกราคม
Maybe not expose this attribute even if the underlying implementation supports it?
This comment also applies for weekday_name (and both in unixtimestampparse).
bake/hclparser/stdlib.go
Outdated
| "iso_week": cty.Number, | ||
| })), | ||
| Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { | ||
| ts := args[0].AsString() |
There was a problem hiding this comment.
No it would fail earlier because an arg is required for this func
| "iso_week": cty.Number, | ||
| })), | ||
| Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { | ||
| ts, _ := args[0].AsBigFloat().Int64() |
There was a problem hiding this comment.
You might want to also handle the error.
There was a problem hiding this comment.
You might want to also handle the error.
.Int64() does not return an error as second value but accuracy: https://pkg.go.dev/math/big#Float.Int64
There was a problem hiding this comment.
See my note for the tests … someone could conceivably supply the following values -1, 0, 1.2, and "0"
| require.Error(t, err) | ||
| } | ||
|
|
||
| func TestUnixTimestampParseFunc(t *testing.T) { |
There was a problem hiding this comment.
You might want to add tests for -1, 0, 1.2, and "0".
bake/hclparser/stdlib_test.go
Outdated
|
|
||
| func TestRfc3339ParseFunc(t *testing.T) { | ||
| fn := rfc3339ParseFunc() | ||
| input := cty.StringVal("2024-06-01T15:04:05Z") |
There was a problem hiding this comment.
You might want to add tests for "" and an syntactically incorrect timestamp.
docs/bake-stdlib.md
Outdated
|
|
||
| ```hcl | ||
| # docker-bake.hcl | ||
| variable "BUILD_TIME" { |
There was a problem hiding this comment.
I don't think this is a convention that we would want to promote. Users should set SOURCE_DATE_EPOCH in the env as UNIX timestamp, not BUILD_TIME as string, so that bake uses the same convention as other apps.
There was a problem hiding this comment.
Indeed, this was just for example purpose, renamed SOURCE_DATE_EPOCH to BUILD_TIMESTAMP.
| * `iso_year` (Number) The ISO 8601 year number. | ||
| * `iso_week` (Number) The ISO 8601 week number. | ||
|
|
||
| ```hcl |
There was a problem hiding this comment.
how about
variable "SOURCE_DATE_EPOCH" {
type = number
default = formattimestamp("X", "2015-10-21T00:00:00Z")
}
target "default" {
args = {
SOURCE_DATE_EPOCH = SOURCE_DATE_EPOCH
BUILD_TIME = formattimestamp("YYYY-MM-DD'T'00:00:00Z", SOURCE_DATE_EPOCH)
}
}
I'm also ok for special formatting for rfc3399 for less verbosity.
There was a problem hiding this comment.
I looked at forking FormatDateFunc func as they are not willing to accept timestamp format: zclconf/go-cty#130 (comment) but there's quite a lot to fork and it might be better to be aligned so users are not confused. So that's why I opted for the terraform unix_timestamp_parse function func instead.
tonistiigi
left a comment
There was a problem hiding this comment.
(looks like github skipped my main review message, reposing in case it doesn't appear)
Still somewhat confused about this.
- what is the use case for
rfc4449parse? We would want to theSOURCE_DATE_EPOCHto be the source of truth that is picked up from env as that is the convention and that is already unix. - Is returning a big struct like these new functions do some kind of convention. Can't find anything similar from current stdlib. The current convention seems to be
formatdate. If we don't want to modifyformatdateto have the capability to take date as UNIX timestamp (and maybe to return it) then would it make sense to add a new function, eg.formattimestampthat is the superset of the existing function (and then hide/deprecate the previousformatdate).
e4913a3 to
12d43b2
Compare
12d43b2 to
af6bc87
Compare
Removed
Yes I think as follow-up we could have a |
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
af6bc87 to
881914b
Compare
fixes #3197
Adds
unixtimestampparsesimilar to terraformunix_timestamp_parsefunction.cc @sdavids