-
Notifications
You must be signed in to change notification settings - Fork 119
misc test fixes #10335
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
misc test fixes #10335
Conversation
enterprise/server/remote_execution/containers/firecracker/firecracker.go
Outdated
Show resolved
Hide resolved
37a4c2b to
bf3b287
Compare
|
still some issues with the permission of the binaries. Gona take a look in a bit |
b1a1eb8 to
ce6e3b2
Compare
|
I dropped the Firecracker changes. It's a bit too complicated to sort out and wouldn't solve my problem (need additional SYS_CAP_ADMIN on jailer). I will skip those |
828aad5 to
b19223f
Compare
| return r.Push(t, image, imageName), image | ||
| } | ||
|
|
||
| func imageFromFiles(files map[string][]byte) (gcr.Image, error) { |
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.
maybe add a comment on why we don't use crane.Image here? Seems likely that someone might try to clean this up/undo the fix by replacing it with crane.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.
Good call. Added.
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.
errrr sorry I should've noticed this before. By overriding all perms to 0o644 I wonder if we aren't testing the "real" tarball perms and this could potentially mask problems. Which test fails without this change and how is it failing?
nvm I think this should be fine
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.
nvm I see - this should only ever be called in cases where the perms actually should be 0o644 anyway. LGTM
1a720e4 to
399090a
Compare
| return r.Push(t, image, imageName), image | ||
| } | ||
|
|
||
| func imageFromFiles(files map[string][]byte) (gcr.Image, error) { |
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.
errrr sorry I should've noticed this before. By overriding all perms to 0o644 I wonder if we aren't testing the "real" tarball perms and this could potentially mask problems. Which test fails without this change and how is it failing?
nvm I think this should be fine
399090a to
05a20a9
Compare
|
Rebasing to re-test everything against latest master before merging |
This PR fixes test which failed locally on my new PC running CachyOS (Arch-based distro).
They are splitted into separate commits for easier review.
cli/parser: stabilize parser tests by isolating HOME
codesearch/github: drop whatchanged for incremental update
runner: materialize disk usage in pool disk limit test
testregistry: build deterministic image fixtures