Skip to content

Conversation

ctrahey
Copy link

@ctrahey ctrahey commented Jun 7, 2025

Related:
#5855
#5216

Description

The method ReplaceRemoteManifestImages is no longer used anywhere except the test case modified in this PR. This is a hint that all manifests, even those from remote, go through ReplaceImages. So this one-line PR exists to demonstrate that a regression has taken place for remoteManifests by showing that it breaks a test case; but is correct to be broken, because the without the change in this PR the test is exercising an orphaned code path.
The expectations outlined in the test case are real expectations that the community relies on for remote manifests.

If I were a better Go programmer, I would love to also PR a fix for this, but I hope this PR make the issue easy to understand.

TL;DR: If this test case were an integration test rather than a unit test, I think it would have started failing 2.5 years ago.

User facing changes

Before:
All tests pass, so we think there is no issue
After:
kubernetes/manifest unit tests fail, revealing the truth about how remoteManifests no longer work

Follow-up Work (remove if N/A)
The handling of remoteManifests needs some tune-up.
Commits in #7603 and #7658 fully removed any actual code paths to ReplaceRemoteManifestImages.

@ctrahey ctrahey requested a review from a team as a code owner June 7, 2025 06:12
@ChrisGe4
Copy link
Contributor

ChrisGe4 commented Jun 9, 2025

Are you trying to use this PR to notify us the ReplaceRemoteManifestImages or ReplaceImages is not working properly? Or is there a specific test case in TestReplaceRemoteManifestImages should be in TestReplaceImages?

I'm having a hard time to understand the change, and the change doesn't seem to be reasonable to me.

@ctrahey
Copy link
Author

ctrahey commented Jun 27, 2025

Its a bit more nuanced than just a notify-via-pr...
The test case I modified here was testing a code path that is not longer reached from anywhere. So the test case was a false positive, but even worse, the expectation that the test case exists to validate is still important - so the fact that my change makes it fail is, in my suggestion, to be taken the same way it should have been taken if the test had started failing back when #7603 and #7658 were open. If they had failed during that work, a fix would likely have been supplied.

@ctrahey
Copy link
Author

ctrahey commented Jun 27, 2025

If it helps clarify, this is my super ugly hack, but this is an absolute pain to maintain across many services:

manifests:
  hooks:
    # This is a hack to work around a defect in Skaffold's rendering of remoteManifests.
    # We're just removing sha digest from the existing deployment because skaffold
    # is accidentally treating remoteManifests the same way as local, where images
    # with digests specified are intentionally skipped.
    before:
      - host:
          command: ["sh", "-c", 'kubectl get deployment -n my-ns -o yaml my-deployment | sed -E "s/^([[:space:]]*image: ${SKAFFOLD_IMAGE_REPO}[^@]*)@sha256.*$/\1/" | kubectl apply -n my-ns --server-side=true --overwrite=true -f -; sleep 5']
          os: [darwin, linux]

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