Unify build strategy fields

[soltysh]

Currently we're starting to copy&paste fields between different build strategies. So far there's almost two of them that are shared between strategies: Image and Env (although the latter isn't present in DockerBuildStrategy). Additionally when @mfojtik PR #1411 lands there'll be DockerRegistrySecretRef. We should move those fields into BuildStrategy.

[mfojtik]

+1 (I don't want to do that in my PR, but rather in separate PR, ideally when #1411 lands ;)

[smarterclayton]

ENV should be in DockerBuildStrategy - we already have use cases for it. We should be clear though that it has different meaning between STI and DockerBuild - in DockerBuild it will be part of the image, vs STI where it's not necessarily part of the final image.

We should make Git repository optional and allow a Docker build if the user just specifies ENV.

[mfojtik]

@smarterclayton how do you get the Dockerfile if you leave Git repository empty?

Or do you wandto make one from baseImage + Env ?

[smarterclayton]

Make one. This isn't super high priority for 3.0, but we eventually want to be able to make Git repo optional and let a user specify a dockerfile in the build config itself.

----- Original Message -----

@smarterclayton how do you get the Dockerfile if you leave Git repository
empty?

Or do you wandto make one from baseImage + Env ?

---

Reply to this email directly or view it on GitHub:
#1412 (comment)

[bparees]

trying to factor out common fields is going to be an on-going headache, i think there's a legitimate argument for having self-contained strategy objects. not to mention the api churn of changing it now. closing, feel free to reopen if you feel strongly and want to debate it.

[bparees]

(what i mean by on-going headache is if we add a new field, we need to either immediately support it in all 3 strategies, or add it in one strategy, and then once we support it for the other strategies, factor the field out of the strategy and move it to the higher level). or add it at the higher level but throw an error if you use it with a strategy that doesn't (yet) support it.