Maintain deterministic order in withCopyFileToContainer and allow file reuse

[findepi]

withCopyFileToContainer can be invoked multiple times, including with
one containerPath being prefix of another containerPath.

This change makes the end result deterministic.

This also allows copying a single source file (e.g. /dev/null) to
multiple destinations.

Fixes #2956

[rnorth]

We're seeing a failure in the API-backwards compatibility tests: unfortunately this PR creates a breaking change in the API (removal of the Lombok-generated accessors for copyToFileContainerPathMap).

Would it be possible to reinstate these accessors, marked @Deprecated and delegating to the replacement code? Sorry that this is likely to be a bit of a faff 😬

Once deprecated, we should remove after people have had a chance to migrate. @bsideup, @kiview please shout if you have any other ideas about how to approach this.

[findepi]

Would it be possible to reinstate these accessors, marked @Deprecated and delegating to the replacement code?

@rnorth sure but let's be specific what exactly do you want them to do.

[rnorth]

Sorry for the delay in getting back to this. I'd assume that we'd be fine to have the ArrayList copyToFileContainerPathList as the only internal representation, with two new accessor methods to replace the generated accessors that have gone away:

getCopyToFileContainerPathMap() - should return a map representation of copyToFileContainerPathList, where last-entry-wins if there are any key collisions.

setCopyToFileContainerPathMap(...) - should accept a map, and overwrite the content of copyToFileContainerPathList with each of the elements in the map. If an ordered map happens to be provided, we should respect the ordering.

Both accessors would be marked @deprecated.

Does this make sense?

[findepi]

@rnorth thanks for looking into this!

setCopyToFileContainerPathMap(...) - should accept a map, and overwrite the content of copyToFileContainerPathList with each of the elements in the map. If an ordered map happens to be provided, we should respect the ordering.

I can implement that

However, calling code may rely on the fact that the map provided setCopyToFileContainerPathMap is the one being used.

For example, today one can write:

Map x = ....
container.setCopyToFileContainerPathMap(x);
x.put(.....)

-- and have effects of put visible.

If we change setCopyToFileContainerPathMap to accept a map and copy values into a list,
we still do not provide backwards compatibility, in a subtle way.

I leave it up to you to decide whether breaking the API in a compile-error manner
would be actually more user friendly.

getCopyToFileContainerPathMap() - should return a map representation of copyToFileContainerPathList, where last-entry-wins if there are any key collisions.

You mean a copy (like Maps.uniqueIndex) or a live view of a list implementing the Map interface?
read-only, or read-write?

[rnorth]

However, calling code may rely on the fact that the map provided setCopyToFileContainerPathMap is the one being used.

Ah yes, you're quite right. In that case, we're running out of options that are not (really) excessive overkill.

The next release (1.15.0) is going to include a swathe of deprecations, and we're allowing some small breaking changes into the API. I think we should go ahead and add this to the list.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

[findepi]

stale, getting this in would fix #2956 and allow me to reduce amount of hack-arounds i maintain.