Remove ApplyLabelSelectorDecorator from MinikubeHandler

[mrizzi]

Fixes #14345

[geoand]

Thanks for this work!

TBH, I don't undestand why your changes is proper fix. I mean what happens if the the application does not provide a custom Deployment, won't the labels be missing?

[mrizzi]

If the user won't provide a custom Deployment, the "application"'s Deployment will be created anyway in the same way it's created when existing resource are provided so basically, at my understanding, using the MinikubeHandler.createDeployment method.
This method adds the selector block invoking createSelector(appConfig) in both cases.
AFAIK, the ApplyLabelSelectorDecorator "forces" the same selector on all the deployments which is not fine when a custom Deployment is provided.

[geoand]

Ah OK, got it.

Things were refactored so I think this makes sense.

@iocanel mind taking a look as well?

[gsmet]

@iocanel ping? I would like to get it in 1.11.1.Final so we would need to merge it by tomorrow evening if you agree it's the right fix.

Thanks!

[iocanel]

I have mixed feelings about it, as there is no guarantee or commitment that new Deployment resources will contain a selector. It's just happens to be currently the case.

We can merge but we need to further improve handling in the future.

[mrizzi]

@iocanel thanks for the info.
I think it's worth having this merged because, for sure, right now the extension has a bug that overwrites the selector: this makes any provided (and working) Deployment broken and not working anymore and hence making the minikube.json file created not working.

[mrizzi]

@iocanel @geoand @gsmet
Sorry to pester you guys but I would like to have this fix for #14345 merged.
The PR contains the test case for the issue I reported.
If it's not "fixing enough" and there's another case to be fixed, could someone provide the test so that we can have this PR to fix everything it's required and then merged?

[geoand]

@iocanel is the absolute expert here, so I'll defer to gimy

[gsmet]

I would tend to agree with @mrizzi here. If it's fixing an issue without introducing any others, I would go for it. We can work on more structural changes later.

@iocanel could you check if we expect this patch to introduce issues or not? If not I would be in favor of merging it for 1.11.2.

[iocanel]

This does fix an issue, without introducing others.
While I am a bit reluctant for the reasons already mentioned, I think that any possible future breakage will be caught by the integraiton tests, that are added. So, lets merge.

[mrizzi]

@gsmet could this be made available in 1.11.2 before 1.12 please?

[geoand]

The backport label has been added so it will be part of 1.11.2

[mrizzi]

Thanks @geoand