-
Notifications
You must be signed in to change notification settings - Fork 576
bake: add unixtimestampparse function #3286
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
base: master
Are you sure you want to change the base?
Conversation
e70b98b
to
7a36d1f
Compare
One uses $ SOURCE_DATE_EPOCH=0 docker buildx bake
$ SOURCE_DATE_EPOCH=42 docker buildx bake Does 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 bake labels = {
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for args
to be empty?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
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.
You might want to also handle the error.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
(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_EPOCH
to 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 modifyformatdate
to 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.formattimestamp
that is the superset of the existing function (and then hide/deprecate the previousformatdate
).
e4913a3
to
12d43b2
Compare
Signed-off-by: CrazyMax <[email protected]>
12d43b2
to
af6bc87
Compare
Removed
Yes I think as follow-up we could have a |
fixes #3197
Adds
unixtimestampparse
similar to terraformunix_timestamp_parse
function.cc @sdavids