Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter out nil addendum before constructing index. #546

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

mattmoor
Copy link
Collaborator

In the PR where we added concurrency we used a fixed length array to store
addendum to preserve the ordering from the base image when constructing the
final index. However, with --platform=... this list may be filtered, which
gives us nil entries in our addendum.

This filters nil entries prior to constructing the index.

Fixes: #544

In the PR where we added concurrency we used a fixed length array to store
addendum to preserve the ordering from the base image when constructing the
final index.  However, with `--platform=...` this list may be filtered, which
gives us `nil` entries in our addendum.

This filters `nil` entries prior to constructing the index.

Fixes: ko-build#544
@mattmoor
Copy link
Collaborator Author

If you have opinions on where I can easily add test coverage for this, I'm happy to, but this fixes @imjasonh repro for me

@mattmoor mattmoor merged commit a56047a into ko-build:main Dec 14, 2021
@mattmoor mattmoor deleted the fix-nil-addendum branch December 14, 2021 19:00
Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

LGTM, but how much do we care about maintaining the original ordering? If we didn't we could just append and then sort for determinism. I guess there's still annoying selection behavior in runtimes if there are multiple matching platforms 🤮

@imjasonh
Copy link
Member

If you have opinions on where I can easily add test coverage for this, I'm happy to, but this fixes @imjasonh repro for me

Adding something to e2e.yaml to just build a two-platform image would be fine by me. We don't even have to run it.

@mattmoor
Copy link
Collaborator Author

Ok added something here: #547

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.

Panic building for multiple explicit platforms
3 participants