-
Notifications
You must be signed in to change notification settings - Fork 527
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
Fail without pushing when crane optimize
misses files
#883
Conversation
39dc2bc
to
063da5a
Compare
Sample failing output (nothing pushed):
Sample successful output:
|
063da5a
to
4625b8a
Compare
@ktock thanks for the new option 🤩 |
crane optimize
misses files
I am trying to pull in containerd/stargz-snapshotter#225 to fix the Windows leg 🤞 |
ef9f50d
to
ace3d93
Compare
Alright, looks like this needs: containerd/stargz-snapshotter#225 to land. |
ace3d93
to
445cffd
Compare
Codecov Report
@@ Coverage Diff @@
## master #883 +/- ##
==========================================
- Coverage 72.76% 72.52% -0.24%
==========================================
Files 108 108
Lines 4655 4670 +15
==========================================
Hits 3387 3387
- Misses 758 773 +15
Partials 510 510
Continue to review full report at Codecov.
|
445cffd
to
958010b
Compare
@imjasonh @jonjohnsonjr this is RFAL (upstream merged, and this is rebased) |
} | ||
missingFromImage = missingFromImage.Intersection(sets.NewString(missingFromLayer...)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the presence of missing files is considered an error, why not just return an error if this set is non-empty? This makes the method a bit easier to use, since you don't have to remember to check for returned missing files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because they might be missing from one image, but not the index.
We only error before pushing because it is only at that point that we have no more places we might find the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. For an index, this only fails if a prioritized file is not found in any of the images. That makes some sense to me, but isn't super intuitive...
Maybe worth logging a warning or requiring a flag that says "yeah I know, these files might not be in every image".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original change only warned for missing files and I found the output easily got lost in the push output, so I opted instead for making it fatal (and not pushing the image(s)).
I do think it would be interesting to add a verbose mode that displays:
- Which files are found in which layers
- Which images are missing which files
... might be useful, but this feels more diagnostic than something I'd want always-on...
TBH I don't know how common different files are for different architectures. I know for muti-arch kaniko (b/c of how Bazel names outputs) I ended up making the key binary /kaniko/executor_{arch}
because it was the only way to get multiple binaries out of Bazel for the same Go 🙃
I suspect that if this takes off, then we're going to want to have significantly more diagnostic tooling around this (e.g. inspecting the TOC of a layer, which files are prioritized?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect that if this takes off, then we're going to want to have significantly more diagnostic tooling around this (e.g. inspecting the TOC of a layer, which files are prioritized?)
Maybe an entire estargz
subcommand? 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hidden, so we can always do that later 🤷
@@ -27,11 +27,12 @@ import ( | |||
"github.com/google/go-containerregistry/pkg/v1/remote" | |||
"github.com/google/go-containerregistry/pkg/v1/tarball" | |||
"github.com/google/go-containerregistry/pkg/v1/types" | |||
"k8s.io/apimachinery/pkg/util/sets" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not super happy about depending on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine :P
} | ||
missingFromImage = missingFromImage.Intersection(sets.NewString(missingFromLayer...)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. For an index, this only fails if a prioritized file is not found in any of the images. That makes some sense to me, but isn't super intuitive...
Maybe worth logging a warning or requiring a flag that says "yeah I know, these files might not be in every image".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not happy about the set thing but it lgtm. When we isolate k8schain (move it or give it its own go.mod), I'll remove this.
No description provided.