Give EnvironmentAndSystemPropertyClientProviderStrategy the highest priority

[kiview]

After merging #4387 and the related fix in #4467, we identified that Testcontainers does not behave as expected in all configuration scenarios, especially after autoIgnoringUserProperties was used in a project.

Therefore, EnvironmentAndSystemPropertyClientProviderStrategy is now tried out as the first strategy, if it is applicable.

This PR also extracts some of the code into variables and private methods, to make reasoning about the order and effect of the ultimate Stream operations easier. The refactorings are just structural changes. I'd recommend checking them in the IDE or the resulting file, instead of the GitHub diff view.

There is probably a bit more opportunity for non-breaking refactorings, but I did not want to blow this PR out of proportion.

In addition, this PR also adds a log for trying out a strategy, to make debugging effective configurations easier.

[rnorth]

This could also be filter, with tryOutStrategy returning a boolean, right?

[kiview]

It could. I kept it like this, because I just made structural changes to the code and extracted methods.

Are you thinking about the boolean to make it more testable or to make the intent cleaner? I would agree that the intent of filter is much clearer.

[rnorth]

Making the intent clearer was my initial thinking, but it would make the function slightly easier to test in isolation as well.

[rnorth]

Sorry: pedantic comment, and I think this comes from the original. Surely this should be findFirst()?

I don't think it's terribly risky, but again it's slightly clearer about the intent.

There's a separate can of wormsopportunities that also opens up when considering that we could try strategies in parallel (but just select the highest priority one that succeeds). I'd not push you to explore that in this PR though :)

[kiview]

Not pedantic at all, but again, that is how the original code does it and I consciously did not touch it.
And I think you are right, this is semantically wrong, since it will not comply with our priorities (although, since we run the stream non-parallel, it is most likely equivalent to findFirst(), but strictly speaking not guaranteed 😨 ).

I am happy to change at least the method in this PR.

[rnorth]

In general I like this PR, BTW - I find it a bit easier to read, and think that it is a good step towards making this more testable.

[kiview]

I added your suggestions, since the change was small and IMO this makes the intent much clearer.

With regards to testing, one approach I see is wrapping the code from tryOutStrategy into an collaborating object, which we can mock/stub for different strategies to allow unit testing. It will still be a challenge, that isApplicable will check for real indicators on the system though.

[rnorth]

Just gave the build a re-run - we may have a flaky test in kafka...

[kiview]

Ok, Kafka is unexpected. All green now.

[bsideup]

nit: do we really need these as variables? 😅 They make it hard to follow what is used where, while if they were arguments of Stream.concat, it would be clear that the only stream we process is orderedProvidedStrategies

[bsideup]

Also, we don't have to use streams here, may as well go for a List of strategies, something like:

var allStrategies = new ArrayList<DockerClientProviderStrategy>();
allStrategies.add(new EnvironmentAndSystemPropertyClientProviderStrategy());

loadConfiguredStrategy().ifPresent(allStrategies::add);

strategies
              .stream()
              .sorted(Comparator.comparing(DockerClientProviderStrategy::getPriority).reversed())
              .collect(Collectors.toCollection(() -> allStrategies));

[kiview]

I disagree, for me it made it much easier to follow like this, that's why I did it.
Introducing those variables gives natural breaking points while reading the code that summarizes what happened so far.

That being said, this is obviously down to personal preference and style as well, so we can proceed either way. It helped me a lot to understand the code and make the changes.

Edit:
Yes, the lists approach looks good, I just did not want to do bigger changes in the PR itself, so I was just restructuring what we had already.

[rnorth]

👍 for the variables for me - it helps convey what these are for

[kiview]

@bsideup
So, should we do this additional refactoring in this PR into using lists instead of Streams? Note that we don't have any explicit tests for this class, so we have to be careful to no accidentally introduce a regression.

[rnorth]

I'd 👍 using lists as well here as well - it's more easily debuggable when looking at the intermediate states.

[bsideup]

@kiview the refactoring to lists is simple and unlikely to introduce any regression, while being a great improvement to the readability / debuggability, so I suggest we do it :)

[kiview]

Alright, will do this.

[kiview]

I think refactoring into lists as an intermediate state made the order much clearer. Can you have another look @rnorth @bsideup ?

Edit:
Flakiness in Kafka test on CI, as @rnorth observed previously.