Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: revert "feat: transform imagePullPolicy when using local cluster (#9495)" #9703

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

plumpy
Copy link
Collaborator

@plumpy plumpy commented Feb 3, 2025

This backs out commit c1834b6 (#9495).

This appears to cause a breaking change, see #9687

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi team,

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

This pull request, submitted by @plumpy, reverses the changes introduced in commit c1834b6296f5f8a19b64c3c97ade0c11e08e6958 which was causing a breaking change (see #9687 for details). The original commit, "feat: transform imagePullPolicy when using local cluster (#9495)", modified the image pull policy when using a local cluster. This PR undoes that modification.

Specifically, the following changes were made:

  • pkg/skaffold/deploy/kubectl/kubectl.go: Lines 244-253 were removed. This section of code dynamically altered the imagePullPolicy in Kubernetes manifests for local clusters. The removal effectively disables this behavior.
  • pkg/skaffold/kubernetes/manifest/image_pull_policy.go: This entire file (88 lines) was deleted. This file contained the logic for selecting and modifying the imagePullPolicy based on resource selectors. Its removal completes the rollback.
  • pkg/skaffold/kubernetes/manifest/image_pull_policy_test.go: This entire file (67 lines) was deleted. This file contained the unit tests for the image_pull_policy.go file. Its removal is consistent with the removal of the functionality it tested.

The intent is to resolve a breaking change introduced by the original feature. The rollback ensures that Skaffold functions correctly without the unintended consequences of the previous implementation.


And now, a little haiku to brighten your day:

Code flows like a stream,
Fixing bugs, a gentle hand,
Peace returns to code.

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request backs out a previous commit that introduced a breaking change. Reverting the commit is the correct approach to address the issue. The changes are straightforward and effectively remove the problematic code. The PR title and description clearly explain the purpose of the revert.

No style guide was provided, so I'm defaulting to the Effective Go style guide for Go code.

@plumpy plumpy changed the title Back out "feat: transform imagePullPolicy when using local cluster (#9495)" feat: revert "feat: transform imagePullPolicy when using local cluster (#9495)" Feb 3, 2025
@plumpy plumpy marked this pull request as ready for review February 3, 2025 21:32
@plumpy plumpy requested a review from a team as a code owner February 3, 2025 21:32
@plumpy plumpy requested a review from alphanota February 3, 2025 21:32
@alphanota alphanota merged commit b212650 into GoogleContainerTools:main Feb 4, 2025
12 checks passed
@plumpy plumpy deleted the revert_9495 branch February 4, 2025 16:10
plumpy added a commit to plumpy/skaffold that referenced this pull request Feb 4, 2025
plumpy added a commit to plumpy/skaffold that referenced this pull request Feb 4, 2025
plumpy added a commit to plumpy/skaffold that referenced this pull request Feb 4, 2025
plumpy added a commit to plumpy/skaffold that referenced this pull request Feb 4, 2025
plumpy added a commit that referenced this pull request Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants