Skip to content

mz334: move resolveCrossStageCommands forwards#337

Open
mzihlmann wants to merge 1 commit intomainfrom
mz334-cache-lookahead-refactoring-part-2
Open

mz334: move resolveCrossStageCommands forwards#337
mzihlmann wants to merge 1 commit intomainfrom
mz334-cache-lookahead-refactoring-part-2

Conversation

@mzihlmann
Copy link
Collaborator

@mzihlmann mzihlmann commented Oct 25, 2025

This is a refactoring in preparation for #334

Description

When we create KanikoStage's we replace all baseImage references with numeric references. But we keep both named and numeric references in COPY --from. ie COPY --from=base or COPY --from=0 (yes this is valid syntax). Later we replace those references a bit hidden as part of ResolveCrossStageInstructions. With this change we just move that transformation to the beginning, so when you have a KanikoStage you are guaranteed to only have numeric references to other stages.

@mzihlmann
Copy link
Collaborator Author

mzihlmann commented Oct 25, 2025

note that this temporarily makes the whole ONBUILD situation worse #332
Maybe we should fix that first as a priority before taking this refactoring. On the other hand I don't expect a single person to actually run into this bug as it appears a pretty useless feature in the spec to me.

Well no, currently you can workaround that issue by not taking any optimizations, ie. --skip-unused-stages=false. If we do this refactoring that is no longer possible and it starts to break for them too. So let's push this out a bit.

@mzihlmann mzihlmann force-pushed the mz334-cache-lookahead-refactoring-part-2 branch from c4959c5 to 21a4da2 Compare October 25, 2025 07:32
@mzihlmann mzihlmann marked this pull request as draft October 25, 2025 07:34
@babs babs requested a review from Copilot October 28, 2025 07:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the handling of cross-stage command resolution by moving the transformation of named stage references to numeric indices earlier in the build process. Instead of resolving COPY --from=<stage_name> to COPY --from=<index> later during ResolveCrossStageInstructions, this transformation now happens immediately when KanikoStage objects are created in MakeKanikoStages.

Key changes:

  • Cross-stage command resolution moved from ResolveCrossStageInstructions to MakeKanikoStages
  • Simplified fetchExtraStages to only handle numeric stage references and remote images
  • Updated skipUnusedStages to remove named reference handling logic

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
pkg/executor/build.go Removed named stage tracking in fetchExtraStages and call to ResolveCrossStageCommands in ResolveCrossStageInstructions
pkg/dockerfile/dockerfile.go Made ResolveCrossStageCommands private, added stage name resolution in MakeKanikoStages, simplified skipUnusedStages
pkg/dockerfile/dockerfile_test.go Updated tests to use MakeKanikoStages and set feature flag environment variable

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mzihlmann mzihlmann force-pushed the mz334-cache-lookahead-refactoring-part-2 branch from f287003 to 456a9cf Compare November 3, 2025 11:05
@mzihlmann mzihlmann force-pushed the mz334-cache-lookahead-refactoring-part-2 branch from 456a9cf to ac9dd38 Compare January 26, 2026 07:10
@mzihlmann mzihlmann force-pushed the mz334-cache-lookahead-refactoring-part-2 branch from ac9dd38 to 0a60a90 Compare February 6, 2026 23:41
@mzihlmann
Copy link
Collaborator Author

as the ONBUILD references are now resolved earlier, we can safely take this refactoring

@mzihlmann mzihlmann marked this pull request as ready for review February 6, 2026 23:43
@mzihlmann mzihlmann requested review from 0hlov3, BobDu, babs and nejch February 6, 2026 23:51
@mzihlmann
Copy link
Collaborator Author

AI summary review:

Overview

Refactoring that moves cross-stage reference resolution (e.g., COPY --from=baseCOPY --from=0) from ResolveCrossStageInstructions to earlier in MakeKanikoStages. This guarantees that KanikoStage objects only contain numeric stage references.

Key Changes

  1. Made function private: ResolveCrossStageCommandsresolveCrossStageCommands
  2. Early resolution: Called during MakeKanikoStages instead of later in pipeline
  3. Simplified fetchExtraStages: Removed stage name tracking logic since all refs are now numeric
  4. Removed fromPreviousStage helper: No longer needed
  5. Cleanup: Removed duplicate error checks in tests

Analysis

✅ Strengths

  • Better architecture: Centralizes transformation logic and makes guarantees explicit
  • Code simplification: Removes ~30 lines of duplicate logic for tracking named stages
  • Improved validation: fetchExtraStages now properly validates stage indices
  • Clean refactoring: Well-structured with appropriate visibility changes

🔍 Considerations

  • Test coverage: Verify existing tests cover both numeric and named COPY --from syntax
  • Backward compatibility: Confirm this doesn't break any edge cases with named stage references
  • Error messages: The new error in fetchExtraStages is clear and helpful

📝 Minor Notes

  • Good cleanup of duplicate testutil.CheckError calls
  • Function visibility change (public→private) is appropriate for internal refactoring

Recommendation

✅ APPROVE - Solid refactoring that simplifies code and improves architecture. No significant risks identified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant