Refactor kaniko builder to cluster builder#2037
Refactor kaniko builder to cluster builder#2037priyawadhwa merged 3 commits intoGoogleContainerTools:masterfrom
Conversation
Custom artifacts will need to work with both the local builder and the cluster builder. However, currently the "cluster" builder is really just the kaniko builder. This refactor renames `pkg/build/kaniko` to `pkg/build/cluster`, and reorganizes it so that it looks like `pkg/build/local`. This way, we should easily be able to add a new CustomArtifact to the cluster builder just as easily as we can to the local builder.
priyawadhwa
requested review from
balopat,
dgageot,
nkubala and
tejal29
as code owners
| } | ||
|
|
||
| // Otherwise, run the builds in sequence. | ||
| return build.InSequence(ctx, out, tags, artifacts, b.runBuildForArtifact) |
There was a problem hiding this comment.
hmm...it's a bit more complicated but I think we should collect the kaniko builds and run them in parallel with building all the custom artifacts in sequence. Building all kaniko artifacts in sequence sounds like a very slow solution and a waste of resources on a cluster.
There was a problem hiding this comment.
Why should be default building custom artifacts in sequence?
Is it because we don't know what the scripts do and they might use same resources and could not run in parallel?
There was a problem hiding this comment.
Something we should consider documenting when we add CustomBuilder.
There was a problem hiding this comment.
With the way this is organized right now, i dont think this is going to used until we support another builder type for in cluster.
Maybe we shd remove it for now? It will help us get our test coverage up too since this branch isn't tested
There was a problem hiding this comment.
Is it because we don't know what the scripts do and they might use same resources and could not run in parallel?
Yah, exactly. There's a section in my DD about letting users define if they want their builds to run in parallel or not, which could be an option.
Am happy to remove it for now and add it back later.
balopat
left a comment
balopat
left a comment
There was a problem hiding this comment.
LGTM except the InParallel vs InSequence comment.
|
@balopat I ended up removing the logic for running in sequence, and I'll add it back when I add the CustomArtifact to the cluster builder. |
…type is supported
Codecov Report
@@ Coverage Diff @@
## master #2037 +/- ##
==========================================
- Coverage 56.09% 56.03% -0.06%
==========================================
Files 175 175
Lines 7607 7615 +8
==========================================
Hits 4267 4267
- Misses 2932 2940 +8
Partials 408 408
Continue to review full report at Codecov.
|
5 participants
Custom artifacts will need to work with both the local builder and the
cluster builder. However, currently the "cluster" builder is really just
the kaniko builder.
This refactor renames
pkg/build/kanikotopkg/build/cluster, andreorganizes
pkg/build/clusterso that it looks likepkg/build/local. This way, weshould easily be able to add a new CustomArtifact to the cluster
builder just as easily as we can to the local builder.