Fixup Docker test

[cachedout]

No description provided.

[ticosax]

Why ?
You broke the meaning of the test. force=True was returning changes before.
This test is here to check no changes are reported in presence of force=True.

Please read in the docstring:

if ``image:latest`` is already downloaded locally the state
        should not report changes.

[rallytime]

@cachedout I had the same question as @ticosax when the fix in #35870 was submitted. The change related to this test failure is in #35581.

[ticosax]

Thank you @rallytime, indeed I missed the notification for #35581 from @cachedout .
As I'm watching the whole project on github, I'm receiving all notifications, and when I'm personally mentioned, I might miss it. That said I setup a new filter to prevent this to happen again.

Now. @cachedout , I'm sorry but from my perspective you didn't manage very well this PR.
When #35581 was submitted, it broke existing tests and you still merged it ! and then with #35892, you hide the regression by corrupting the failing test !! It is beyond any kind of reasonable logic for me. The assumption I make is that you might be overwhelmed, tired and maybe close to burnout. You are a really talented developer, it is not the quality I used to see from your contributions and your implication in the project.

It is frustrating because I wrote probably wrote more than 90% of tests for dockerng, specifically to prevent this kind of regression. That's my working time that is involved and overall saltstack is such a critical part in our infrastructure and our business that this kind of behavior is a red flag that tells me: Runaway !!!

Now let's talk about use case raised by #35581.
I believe we shouldn't go down the same road that docker took. e.g. In case of missing repository name in the image name, assume the repository is docker's hub. This assumption is a bad design choice (explicit is better than implicit) and error prone. (I didn't look at the internal of docker for that but I assume it is not pretty with different hardcoded hostnames). Instead we should document that we support only full image names.

Even more, because of #35581, if a user ask to pull alpine:edge any image that ends with alpine:edge will be returned, even if the local image is called evil-repo/apline:edge... You understand the implication of such algorithm. This is why I don't even want to fix it myself, the repercussions might be really really bad.

I'm going to submit a revert PR for #35581 and #35892. And submit another PR to document we support only full namespaced image names.

And please reinsure me that quality is a priority for saltstack project and PR that are breaking tests shouldn't be allowed to be merged so easily. In regard of the problem being solved by #35581, I don't think it even worth the time invested.

[cachedout]

Hi @ticosax

Thanks for the feedback. A few points.

You're completely right about this being a mistake. Unfortunately, I can't know everything about every piece of Salt and the Docker module is an area where I know less. (Mostly because it's typically so well-maintained by you and by others, which I'm grateful for.) That's the nature of a project like this. I'd be pretending if I (or any developer or project maintainer) was an expert about every piece of code inside Salt.

As you certainly know, I typically ping you on Docker-related changed. As you pointed out, I did do that on #35581 but you must not have seen it since I didn't get a reply from you.

This happens from time to time with contributions. Typically, if the PR is in develop and I don't get an answer, I'll end up merging it.

This isn't because I don't care about quality. (I very much do.) It's because the purpose of the develop branch, as I see it, is to get code into Salt so that others can start working on it, fixing bugs that might be present, etc.

If we waited until every PR was completely perfect before we merged it, as nice as that sounds, we'd be completely overwhelmed and the project would be virtually impossible to manage, given our current resources. There might be ways around that, but it's a larger discussion.

As for the test, I mostly agree with you. I certainly wasn't attempting to cover up the regression. As I read the code and looked at the new behavior, it seemed like the adjustment that needed to be made to align the test with a behavioral change. It would be nice if every test were a hard and fast safeguard against regressions.

However, as I'm sure we all know -- there are times when the reverse of what you're saying is true and the test itself it passing with broken behavior and the proper fix breaks the test. It's a judgement call about which is the case and as you've mentioned, in this case the wrong one was made. I apologize for that, certainly. I was working under the false assumption that since you missed the ping on the original issue that you might not be around to look at the test failure. Clearly, that was a bad judgement on my part.

As for my own burnout, I do work (generally) work 12+ hours a day on Salt and I haven't had a day that I haven't worked since late July, so I do confess to being tired. We're doing the best we can with the resources that we have. I really do appreciate your concern and for pointing out when mistakes are made.

Thanks also for the fixes incoming. I'm glad we could get them all addressed and cleaned up.

Cheers, -mp

[ticosax]

Thanks @cachedout I didn't realized the develop branch was such a playground.
I'll remind it and try to be less dramatic if it happens again.

Cheers