While updating; I see that rootfs is shadowing the rootfs import; perhaps change to rootFS

Oh! Actually, that won't work either, because there's already a variable rootFS. Hm.. that's confusing for sure! Looks like we may want to look at variables used, because it's really easy to pick the wrong one then?

Thanks. I opted for aliasing the rootfs import to rootfspkg, to avoid confusion with the rootfs variables in the code.

Avoid renaming imports unless the name conflicts with another import. https://go.dev/wiki/CodeReviewComments#imports

Had a quick peek; looks like this was added in 22f7cae, and never used. Assuming the current implementation won't change, I don't think there's much need for a context, so probably fine indeed to remove.

Looks like this was used in the past, but its use was removed in e51aec9, so looks fine to remove;

• Don't append the container id to custom directory checkpoints. #35694

Looks like the context here was added in cad9713, which at the time also called i.client.ImageService().List(ctx, "target.digest=="+imgID.String())

That code was removed in 87c87bc, however it added a TODO;

Not 100% what's needed to satisfy that TODO, but perhaps we should check if that may involve a context to be passed; in that case it may be better to keep the context, otherwise we end up removing contexts that later have to be reintroduced again.

Thanks; my view is: I don't think we should keep unused variables (ctx in this case) unless there's a compelling reason to do so (e.g., such as it's part of an existing API or interface, or having the variable be unused is clearly a bug). Otherwise it's confusing when folks read the code, plus it leads to unused variables that never get removed.

In this particular case, the checkImageDeleteConflict function is just an internal function (not an API or interface), and therefore I think it's best to remove the unused variable. If and when the "TODO" you mentioned is done and if a context is needed, then it will be plumbed accordingly (and that should be trivial to do).

Context variables will be used if the function is annotated with logging (e.g. log.G(ctx).Warn(...)) or OpenTelemetry tracing events. Contexts are for propagating contextual metadata about the function call, not just cancelation and deadlines.

Why not add the context variable if and when the function needs it though? Otherwise we have a unused variable that may remain there forever.

Otherwise we have a unused variable that may remain there forever.

So what?

Changing a function signature requires touching all the call sites. That's a tedious refactor that can't be automated: whether the parameter value should be ctx, context.Background(), context.TODO() or context.WithoutCancel(ctx) depends on what's calling the function. Having the context parameter already roughed-in to the function signature allows authors to set the parameter value appropriately when writing the code that calls the function instead of leaving it to the poor soul who has to change the function signature and fix up all callers.

Preemptively adding context parameters to functions is a pragmatic decision which favours maintainability and sustainable development over aesthetics.

You're both right on this one!

In general, I agree with @ctalledo try to prevent implementing things ahead of time (risk of YAGNI!), unless you're really sure it's gonna be used (in which case probably a tracking ticket and/or TODO are warranted). And perhaps some of these cases where a context.Context was added were pre-maturely.

That said; context.Context is a bit of an outlier; we are still in process of modernizing our code-base to make more parts context-aware and/or adding telemetry (and context logging). For some of those it's worth the trade-off of having some signatures already include a context; having the context in the signature is a good reminder for those working on the code to consider adding context-handling.

There's various places where preparations have been made to wire up the context, and removing the unused context, even from a non-exported function can sometimes cause a ripple effect; remove it from the un-exported function, and next someone comes on functions that call the un-exported function, and may now be compelled to remove the context from that as well (potentially undoing the preparations).

Is it perfect? No it's not!
Are there cases where context were added too premature? Possibly!

In this case; I noticed the TODO I linked to above, and I'm tempted to say "It depends" - I don't know (yet) what's involved in addressing that TODO, and it's possible that addressing it involves calling a function that expects a context, and for that I definitely agree with @corhere - it's a minor burden to keep it in place, and probably more disruptive if we have to re-wire the contexts if it turns out we need it.

OK I left it in place in this function, though I don't see a need to keep it in the getLayers() function above.

@corhere: it's not an aesthetic concern, having unused parameters is confusing to the reader.

I suspect that the metaHeaders here are something that's not implemented yet for the containerd image store. Looking at the equivalent code for non-containerd, it looks like this is wired up;

So this actually looks like a bug, or at least "something's missing" and we may not have a tracking ticket for it (but we should?);

In this case the fact that metaHeaders is unused does seem like a clear bug, given that it's used in the docker image store implementation of the PushImage interface. So I've added it back.

Having said that, should we file a ticket to investigate further and actually use the metaHeaders in the implementation of PushImage for the containerd image store?

This looks like a bug, or something left to implement

Yes agreed, I've reverted the change.

Related to the other comment; looks like this is something not yet implemented, and for which we may need a tracking ticket.

If I'm not mistaken; options available here may depend on the storage driver used (with graph-drivers), and quite likely we currently don't have options yet for the containerd snapshotters (but there's some options, such as size that may be relevant, but perhaps not yet supported by containerd?

Got it, I've reverted that change too.

which we may need a tracking ticket

Who would be best to file such a ticket (I am afraid I don't understand the options for containerd-snapshotter well enough to write to create the description of the ticket)?