Skip to content

Conversation

@mzihlmann
Copy link
Collaborator

@mzihlmann mzihlmann commented Oct 12, 2025

Fixes #305

Description

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

Release Notes

Describe any changes here so maintainer can include it in the release notes, or delete this block.

Examples of user facing changes:
- kaniko adds a new flag `--registry-repo` to override registry

@mzihlmann
Copy link
Collaborator Author

mzihlmann commented Oct 12, 2025

I think the best course of action is to move the skipUnusedStages later down in the process. instead of working on the input instructions.Stage we can work on the "intermediate" unoptimized config.KanikoStage instead. The latter contains an Index field already. If skippings happens after this index field is filled-in, it will contain the correct info for the rest of the build. We just need to make sure that everywhere where store stages, we don't use the actual list-index, but the index property of the element instead and this bug should be resolved nicely.

This also aligns nicely with my plans to solve #299. As there we need to run skipUnusedStages on config.KanikoStage anyways, as this time we have to run even later, after the cache optimizations kick in, so the same code can be reused too, which is nice.

This also simplifies the body of the function a bit, as in the process of converting to kaniko-stages, we convert all named references to numeric references, even for FROM (where there is no syntax support). This means we no longer need to handle both cases and the errors that ensue with conversion, the conversion is already done once, prior to the logic, which is also a win.

@mzihlmann
Copy link
Collaborator Author

hmmm I think the latter is not true, the references are not replaced with numeric references yet, that follows later.

@mzihlmann mzihlmann force-pushed the mz305-update-numeric-references branch 3 times, most recently from c5b3fba to 3935917 Compare October 12, 2025 11:43
@mzihlmann mzihlmann changed the title mz305: integration test mz305: skip-unused-stages invalidates numeric references Oct 12, 2025
@mzihlmann
Copy link
Collaborator Author

I noticed that the pre-build skirmishes are written super confused, we go back and forth talking about stages by name, then by index, creating the same mappings over and over again, skipping stages happens first, then we resolve dependencies again later on etc etc. Code quality wise there is a lot to be gained by just cleaning that mess up. I'm thinking about spending at least one file for all the build graph calculations & optimizations, now the functions are distributed and duplicated all over the place.

@mzihlmann mzihlmann force-pushed the mz305-update-numeric-references branch from 3935917 to 741eb97 Compare October 12, 2025 11:55
@mzihlmann mzihlmann marked this pull request as ready for review October 12, 2025 11:55
@mzihlmann
Copy link
Collaborator Author

The last part of the unittests can be dropped completely. We no longer change the stage indizes, so we also don't need to test that they remain the same.

After skip & squash the index in the list of stages can change, but references to the stages are not updated.
We fix this by storing the original index on the element and only refering to the stage using it's original index.
For that end the skipUnusedStages was moved to work on the "intermediate" kanikoStages.
@mzihlmann mzihlmann force-pushed the mz305-update-numeric-references branch from 741eb97 to ed7f31e Compare October 12, 2025 13:29
@mzihlmann mzihlmann merged commit 6185cbb into main Oct 12, 2025
10 checks passed
@mzihlmann mzihlmann deleted the mz305-update-numeric-references branch October 19, 2025 07:07
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.

skip-unused-stages invalidates numeric references

2 participants