fix: imagePullPolicy only changed to 'Never' if 'Always' is specified.

[alphanota]

Fixes: #9687

Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs

Description
This PR fixes the regression introduced in PR#9645.
The PR added logic to set the value of the 'imagePullPolicy' field in k8s manifests to Never, if the field was present. This caused an error if the field was set to IfNotPresent - the new code set the value to Never and caused the deployment to fail if the required image was not already present locally and needed to be pulled.

I amended the behavior so that the imagePullPolicy field is set to 'Never' only if the value read is equal to 'Always' anything else is left alone.

I think we lacked integration tests where the imagePullPolicy field in k8s manifests was specified so this is why [PR#9645] passed all tests, since the code only activates if the imagePullPolicyField is specified. I added integration tests to ensure the expected behavior when the field is specified

User facing changes (remove if N/A)

This PR fixes bug where images are not downloaded even if they are not present locally and the image pull policy is set to pull if not present.

[code-review-assist]

Hi team,

I'm currently reviewing this pull request and will provide my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly understand the changes:

This pull request, authored by alphanota, aims to refine the handling of the imagePullPolicy in Kubernetes manifests. Specifically, it ensures that the policy is only set to 'Never' if it was originally specified as 'Always'. This prevents unintended side effects when using Skaffold's manifest manipulation features.

Here's a breakdown of the changes:

• Added new integration tests: A new set of tests (TestRunWithImagePullPolicy) has been added to integration/run_test.go (lines 235-287) to thoroughly test the different imagePullPolicy scenarios ('Never', 'Always', 'IfNotPresent'). These tests involve building docker images within minikube (lines 267-273) and using exec.Command (line 270) to run minikube commands. Additional test data directories were created under integration/testdata/image-pull-policy/ to support these tests (lines 1-9, 1-7, 1-9, 1-7, 1-9, 1-7).
• Modified image_pull_policy.go: The core logic for setting the imagePullPolicy has been updated in pkg/skaffold/kubernetes/manifest/image_pull_policy.go. The constants imagePullPolicyField, imagePullPolicyAlways, and imagePullPolicyNever were added (lines 21-23). The Visit function in imagePullPolicyReplacer now only sets the policy to 'Never' if the original policy was 'Always' (lines 89-91). The unnecessary const imagePullPolicyField declaration within the Visit function was removed (line 80).
• Updated a test case: A test case in pkg/skaffold/kubernetes/manifest/image_pull_policy_test.go (line 53) was updated to reflect the new behavior.

The changes are well-structured and the added tests provide comprehensive coverage of the updated logic. I'll be checking for any potential regressions or edge cases during my full review.

---

And now, a little haiku to lighten the mood:

Code flows like a stream,
Pull requests, a gentle tide,
Bugs swim to the shore.

[code-review-assist]

This pull request fixes a bug where the imagePullPolicy was only changed to Never if Always was specified. The implementation looks good and the added tests cover the different scenarios well. I've summarized the Google Go Style Guide elements referenced in this review below:

• Naming Conventions: Used imagePullPolicyField, imagePullPolicyAlways, and imagePullPolicyNever to adhere to constant naming conventions.
• Testability: Added integration tests to verify the fix.

Overall, the changes are well-structured and easy to understand. I recommend adding a brief entry to the project's README (if one exists) to document this behavior change for users.

[plumpy]

import k8s "k8s.io/api/core/v1"

and then you can use k8s.PullIfNotPresent and PullAlways