-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix: remote manifest image substitution #6342
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6342 +/- ##
==========================================
- Coverage 70.57% 70.48% -0.10%
==========================================
Files 496 498 +2
Lines 22457 22581 +124
==========================================
+ Hits 15850 15917 +67
- Misses 5584 5633 +49
- Partials 1023 1031 +8
Continue to review full report at Codecov.
|
// if manifest mentions image with repository then `imageName` is parsed into `image.Name` | ||
tag, present := tagsByImageName[image.Name] | ||
return image.Name, tag, present |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I'm reading this correctly, this block is to account for the existing locally-referenced images, right?
if so, it seems that this block will always be used in selecting the images to replace. it might be a little easier to read to remove this part from the selectRemoteManifestImages()
function, and then alter the if len(k.remoteManifests) > 0 ... else
block to just be an if len(k.remoteManifests) > 0 ...
.
if len(k.RemoteManifests) > 0 {
manifests, err = manifests.ReplaceRemoteManifestImages(ctx, builds)
}
manifests, err = manifests.ReplaceImages(ctx, builds)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made the separation between local and remote manfests cleaner. ReplaceRemoteManifestImages
still needs to account for images referenced directly similar to local manifests.
Fixes: #5855
Description
Skaffold expects local kubernetes manifests to define
containers[i].image
property exactly equal toskaffold.yaml
'sartifacts[i].imageName
without a digest suffix or repository prefix. This works forkubectl.remoteManifests
deployer only with local clusters like minikube or kind where the image is referenced without digests.This PR separates the image substitution logic between
localManifests
that need the image name to match artifactimageName
exactly; andremoteManifests
that can specify images with digest and repository.Testing instructions
In
examples/getting-started
:skaffold run
skaffold dev
main.go
to print something else