Remove build dep for helm deploy #2121
Conversation
On master, skaffold deploy fails with following error. ``` skaffold deploy -f examples/helm-deployment/skaffold.yaml Starting deploy... Error: release: "skaffold-helm" not found Helm release skaffold-helm not installed. Installing... FATA[0002] deploying skaffold-helm: matching build results to chart values: no build present for gcr.io/k8s-skaffold/skaffold-helm ``` This is because in GoogleContainerTools#922, we removed deploy triggering the build. Running deploy should use the default tag i.e. "latest" when depoying images.
tejal29
requested review from
balopat,
dgageot,
nkubala and
priyawadhwa
as code owners
Codecov Report
@@ Coverage Diff @@
## master #2121 +/- ##
==========================================
+ Coverage 56.29% 56.76% +0.46%
==========================================
Files 180 183 +3
Lines 7794 7864 +70
==========================================
+ Hits 4388 4464 +76
+ Misses 2989 2988 -1
+ Partials 417 412 -5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I wonder - are certain error classes going to be discovered later?
Specifically, if the helm values in the helm release config are not matching up with the artifact name due to a typo - are we just going to print a warning and fail during deployment time as we are assuming that the image was built and pushed?
Can we somehow discern between the two modes? i.e. when builds are empty (~ we are in deploy mode) then we don't check for matching, but if we are in dev/run (we do build) then we are?
|
@balopat Thats a good point! |
tejal29
dismissed
balopat’s stale review
spoke with balint and he said the changes looked good.
pkg/skaffold/deploy/helm.go
Outdated
| if !ok { | ||
| return nil, fmt.Errorf("no build present for %s", imageName) | ||
| if len(builds) == 0 { | ||
| logrus.Warnf("no build artifacts present. Assuming skaffold deploy. Continuing with %s", imageName) |
There was a problem hiding this comment.
I would make this a Debug statement instead.
There was a problem hiding this comment.
I would actually just remove this altogether.
if len(builds) > 0 {
return nil, fmt.Errorf("no build present for %s", imageName)
}
b = build.Artifact{ImageName: imageName, Tag: imageName}There was a problem hiding this comment.
@nkubala I wanted to print a warn or a debug message to just give users more insight in to what is happening.
There was a problem hiding this comment.
right, this would do that. I'm just proposing that you remove the log message since I don't really think it's necessary. the change I proposed is logically equivalent, just a bit easier to read IMO.
if we didn't find the build in the build map:
* if build map is not empty -> error out because we couldn't find the specified artifact
* continue, because we want to allow deploying without anything being built
pkg/skaffold/deploy/helm.go
Outdated
| if !ok { | ||
| return nil, fmt.Errorf("no build present for %s", imageName) | ||
| if len(builds) == 0 { | ||
| logrus.Warnf("no build artifacts present. Assuming skaffold deploy. Continuing with %s", imageName) |
There was a problem hiding this comment.
I would actually just remove this altogether.
if len(builds) > 0 {
return nil, fmt.Errorf("no build present for %s", imageName)
}
b = build.Artifact{ImageName: imageName, Tag: imageName}
On master, skaffold deploy fails with following error.
This is because in #922, we removed deploy triggering the build. Running
deploy should use the default tag i.e. "latest" when depoying images.