Remove PreBuiltImageBuilder and make it a function instead.

[tejal29]

For #922, i am making changes to skaffold deploy to not run skaffold build.
--images flag is only used deploy flow.
The way it works is when --images flag is present, runner creates a
PreBuiltImageBuilder. All it does is, parses the images and converts
them to build.Artifact. If during deploy, an image in the list was also
built, then it uses the built image tag vs the tag present on the
commmand line flag.

In this change, we remove creating a PreBuiltImageBuilder in the runner.
(Remember this flag is only available for deploy and hence other command
won't get affected)
The --images will now be converted to build.Artifact in the
runner.Deploy flow.

[priyawadhwa]

LGTM

[codecov-io]

Codecov Report

Merging #1986 into master will decrease coverage by 0.16%.
The diff coverage is 28.12%.

@@            Coverage Diff             @@
##           master    #1986      +/-   ##
==========================================
- Coverage   55.48%   55.32%   -0.17%     
==========================================
  Files         173      173              
  Lines        7510     7505       -5     
==========================================
- Hits         4167     4152      -15     
- Misses       2952     2961       +9     
- Partials      391      392       +1

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/deploy.go	34.61% <0%> (-40.39%)	⬇️
pkg/skaffold/runner/runner.go	61.93% <0%> (-1.08%)	⬇️
pkg/skaffold/runner/prebuilt.go	81.81% <81.81%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37baddd...71b52b6. Read the comment docs.

[dgageot]

The idea behind curent implementation is to remove as many branches as possible in the Runner's functions. Too many branches make it too hard to maintain. I'd like to see the bug fixed without removing the PreBuiltImageBuilder

[tejal29]

@dgageot, Keeping PreBuiltImageBuilder adds unnecessary build flow into deploy which we need to get rid off.
Other way to fix this, by introducing one more flow in the runner where if command is deploy and opts.PreBuiltImages is present, then run build or else only deploy which is add more complexity to the code.

[balopat]

The idea behind curent implementation is to remove as many branches as possible in the Runner's functions. Too many branches make it too hard to maintain. I'd like to see the bug fixed without removing the PreBuiltImageBuilder

I agree that introducing an if statement and overriding the artifacts parameter in the Deploy method is not the nicest, it would be great to have the Build step be conditionally skipped in case we are using prebuilt images and pass in the artifacts to be deployed to the Deploy method.

This conditional branching is implemented currently by the switch statement used in the Build step. Which is - I think one of those "clever" solutions that are a bit of a surprising as

• for skaffold deploy we are doing a build - and led to the bug of introducing a build step during a deploy-only operation.
• we are "building prebuilt images"... which is just weird

So I think we can do better than the current design in terms of non-surpising, more maintainable code.

My suggestion would be to introduce the explicit conditional in the runner above the build step.