vendor.mod: github.com/microsoft/hcsshim v0.12.5#48174
Conversation
AkihiroSuda
force-pushed
the
hcsshim-0.12.5
branch
from
3f1ba2e to
ac83d81
Compare
AkihiroSuda
changed the title
|
Ah, yes, I recall I did not yet update RootlessKit as I wasn't sure if updating hcsshim to a version that didn't match containerd would be possibly problematic (containerd 1.7 is still on v0.11; https://github.com/containerd/containerd/blob/v1.7.19/go.mod#L10 |
thaJeztah
added
status/2-code-review
area/rootless
AkihiroSuda
force-pushed
the
hcsshim-0.12.5
branch
from
ac83d81 to
f29dbdf
Compare
|
ping @thaJeztah @dmcgowan @kiashok 🙏 |
AkihiroSuda
force-pushed
the
hcsshim-0.12.5
branch
from
f29dbdf to
468285e
Compare
|
@AkihiroSuda I opened #48283 to get those in already, while we get confirmation from the others |
| CreatedAt: p[i].CreateTimestamp, | ||
| CreatedAt: timestamppb.New(p[i].CreateTimestamp), |
There was a problem hiding this comment.
Oh; didn't make this change in the other PR; was this one related to the hcsshim update or to the grpc/proto updates?
There was a problem hiding this comment.
Seems related to hcsshim
# github.com/docker/docker/libcontainerd/local
..\..\libcontainerd\local\local_windows.go:944:34: cannot use p[i].CreateTimestamp (variable of type time.Time) as *timestamppb.Timestamp value in struct literal
| err1 := RemoveAllRelative(path+string(os.PathSeparator)+name, root) | ||
| if err == nil { | ||
| err = err1 | ||
| if err2 := RemoveAllRelative(path+string(os.PathSeparator)+name, root); err == nil { | ||
| err = err2 |
There was a problem hiding this comment.
Was glancing over changes in the update, and not a fan of formatting this code like this; the previous formatting made it much less error-prone;
- it makes it really easy to read the code to be checking for
err2, but it's not. - it makes it really easy to think it's checking for non-
nilerror, but it's not.
And the overall use of a err as global, reused variable is a real risk of that variable being accidentally shadowed. I wonder if this code can be rewritten with early returns or otherwise.
There was a problem hiding this comment.
Could you open an issue in the hcsshim repo?
There was a problem hiding this comment.
Yup; was planning to open a ticket or PR (definitely not an issue with your PR, but I stumbled upon it)
AkihiroSuda
force-pushed
the
hcsshim-0.12.5
branch
from
468285e to
fe13f55
Compare
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
sorry forgot about this one; per the discussion in the containerd slack, it looks like this is probably fine to be ahead of containerd.
Let me double check with some other folks, but otherwise I think this should be OK to get in.
|
Can we merge? |
AkihiroSuda
force-pushed
the
hcsshim-0.12.5
branch
from
fe13f55 to
cdfd2af
Compare
microsoft/hcsshim@v0.11.7...v0.12.5 Signed-off-by: Akihiro Suda <[email protected]>
AkihiroSuda
force-pushed
the
hcsshim-0.12.5
branch
from
cdfd2af to
f49fad7
Compare
5 participants
microsoft/hcsshim@v0.11.7...v0.12.5