containerimage: support SOURCE_DATE_EPOCH for CreatedAt #3263
Conversation
AkihiroSuda
force-pushed
the
fix-createdat-epoch
branch
from
c6a5ba9
to
29d8534
Compare
AkihiroSuda
force-pushed
the
fix-createdat-epoch
branch
from
29d8534
to
b339dc3
Compare
exporter/containerimage/export.go
Outdated
| Target: *desc, | ||
| CreatedAt: time.Now(), | ||
| Target: *desc, | ||
| // CreatedAt is ignored (propagated via imageClientCtx) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Why? You are saying that there is a field CreatedAt that Create() and Update() methods take, but the implementation is broken, and it doesn't work. And instead, only way to pass the value is with this context dance?
There was a problem hiding this comment.
Yes, but probably s/broken/unsupported by design/.
The context was chosen to keep the Go API unmodified.
There was a problem hiding this comment.
What should we do with this?
There was a problem hiding this comment.
Is there a containerd issue tracking this? It looks like a bug, and we are trying to work around it.
keep the Go API unmodified.
I don't see how this even matters in this case. The value is inside a struct, so it doesn't change the function signature, and the value has already been there(and it looks like we were even setting it).
There was a problem hiding this comment.
It looks like a bug
Doesn't seem so IIUC: containerd/containerd#8225 (comment)
it doesn't change the function signature
Still CreatedAt time.Time in the struct would have to be changed to CreatedAt *time.Time to differentiate "unset" from "1970-01-01 00:00:00".
Alternatively we could add CreatedAtIsUnset bool in the struct, but that might look uglier than ctx.
There was a problem hiding this comment.
Doesn't seem so IIUC: containerd/containerd#8225 (comment)
I don't think that contradicts. Yes, this is not a build time, but just some time value associated with the containerd image store. But it doesn't change how the value is updated. This is the same value that is returned when querying the Image struct.
Alternatively we could add CreatedAtIsUnset bool in the struct
I think I would prefer this. It is not a blocker for this PR, but we should have something in containerd that tracks this issue so it doesn't get forgotten.
There was a problem hiding this comment.
Opened an issue for continuing the discussion:
There was a problem hiding this comment.
Can we mark more clearly that this is a containerd bug in a comment and needs a follow-up after a fix is done there. There has not been much movement in the containerd issue. I'd like to be sure we are not stuck with this solution forever.
| // https://github.com/containerd/containerd/commit/133ddce7cf18a1db175150e7a69470dea1bb3132 | ||
| integration.CheckContainerdVersion(t, cdAddress, ">= 1.7.0-beta.1") |
There was a problem hiding this comment.
Can't be removed, as we still test containerd 1.6 too
|
@tonistiigi PTAL |
|
What should we do with this for now? |
AkihiroSuda
force-pushed
the
fix-createdat-epoch
branch
from
b339dc3
to
4b67818
Compare
|
Rebased |
Fix issue 3167 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
AkihiroSuda
force-pushed
the
fix-createdat-epoch
branch
from
4b67818
to
601dd5b
Compare
Fix #3167