Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the KanikoStage data structure to better separate concerns between parsing, transformation, and optimization phases. The key change is removing the embedded instructions.Stage from KanikoStage and replacing it with explicit fields (Name, BaseName, Commands). This makes the structure more explicit about what data it contains and when transformations (like resolving cross-stage references) are applied.
Key Changes:
- Refactored
KanikoStageto contain explicit fields instead of embeddinginstructions.Stage - Moved cross-stage command resolution into
MakeKanikoStagesfunction for clearer transformation boundaries - Changed stage index mapping from
map[string]stringtomap[string]intthroughout the codebase
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/config/stage.go | Modified KanikoStage struct to use explicit fields instead of embedding instructions.Stage |
| pkg/dockerfile/dockerfile.go | Moved cross-stage resolution into MakeKanikoStages, changed stage mapping to use int instead of string, updated squash function |
| pkg/executor/build.go | Updated stageBuilder and related functions to work with new KanikoStage structure, changed stage index maps from string to int |
| pkg/executor/build_test.go | Updated test code to construct KanikoStage with explicit fields, fixed field references in tests |
| pkg/image/image_util_test.go | Updated test instantiation of KanikoStage to use explicit fields |
| pkg/dockerfile/dockerfile_test.go | Updated tests to use MakeKanikoStages instead of manual construction, fixed case sensitivity in test strings |
Comments suppressed due to low confidence (1)
pkg/executor/build.go:1
- This line removes a call to
ResolveCrossStageCommandsthat was previously executed inResolveCrossStageInstructions. Since cross-stage resolution has been moved toMakeKanikoStages(called earlier inDoBuild), this removal is correct. However, the functionResolveCrossStageInstructionsnow only builds the name-to-index map without actually resolving commands, which contradicts its name and could confuse future maintainers.
/*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1a9d2f8 to
c358749
Compare
c358749 to
f214011
Compare
f214011 to
da8dbe4
Compare
|
With the other PRs now merged the scope of this PR is reduced drastically. All it is concerned with is simplifying the datastructures so we don't carry the input data around as a field on the output data, but instead carry over all the fields we need into a flat output structure. By getting rid of this nesting we can fight the "gorilla jungle problem", where you ask for a banana, and you have a method to get a banana, but it's in the form of a gorilla holding the banana and the entire jungle alongside with it. It's a common source of confusion, keeping the datastructures minimal is hence advisable. |
Description
To enable cache lookahead we need to redefine some of the datastructures we use during build. The idea is to move transformations forwards and optimizations backwards, s.t. ultimately we can do cache elimination over all stages prior to build. Here we only do the refactoring, nothing should change on the functional level.
The core-idea is to make
KanikoStagemore potent. Currently it starts out as a dockerfile stage (parser result) and gradually gets modified to become more useful and finally gets converted into a stagebuilder struct. The problem I have with this that at different points in the build flow, the fields in kanikostage serve different functions. The most gross violation I think is that at some point theCOPY --from=baseget replaced with the numericCOPY --from=0. Which, don't get me wrong, I think is a good transformation, but I think it should happen directly when we convert tokanikoStagenot at some point later, s.t. the programmer has to be aware which transformations were already done and which weren't.Another transformation is that on-build commands get merged into the regular commands, this just happens at some point, let's all move it into the
makeKanikoStagesfunction, s.t. it is a clear cut transformation.Optimizations on the other hand, which currently are in that function, should imo. happen outside and afterwards.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Reviewer Notes
Release Notes
Describe any changes here so maintainer can include it in the release notes, or delete this block.