Conversation
|
@majst01 okay, so this is what I came up with. Can you do a review, pls? Besides, I'm wondering if it's suitable to retrieve the oci credentials from env vars. Can you help me out here? |
The OCI creadentials should be passed as optional fields from the pixiecore to the metal-hammer: https://github.com/metal-stack/metal-hammer/blob/master/cmd/pixie-client.go#L14 Ideally a slice of registryconfig which contains the registry url, the username and the password should come from pixiecore. |
|
@majst01 does this work? https://github.com/metal-stack/metal-hammer/pull/175/files#diff-cfcd5a78ad9476ee24d7680dd023e71136f329c771d76ae6837ba68320b6379cR37 what does |
It contains the full URL as configured in the image |
mac641
force-pushed
the
oci-puller
branch
3 times, most recently
from
0bfe689 to
7052ccb
Compare
machine.Allocation.Image.URL can be used without auth
since it's not supported by name.ParseReference
…nd default to image
mac641
force-pushed
the
oci-puller
branch
7 times, most recently
from
ad2bb15 to
2506148
Compare
mac641
force-pushed
the
oci-puller
branch
2 times, most recently
from
57274af to
ffebda3
Compare
since they're considered integration tests and can't be run inside a docker container due to testcontainers-go
| } | ||
|
|
||
| switch hdr.Typeflag { | ||
| case tar.TypeDir: |
There was a problem hiding this comment.
Unsure if all cases are covered and or relevant.
Other pitfalls might be:
- files with s-bit set
- file modes of special files like /etc/shadow /etc/passwd etc.
This should be tested, at least manually, better would be with a automated test ?
…ct skipped files - reduce code duplication in untar func
|
@majst01 So, I've been adding a couple of tests, mainly targeting s-bits and symlinks but it's impossible to test special files like |
| i.log.Info("log image ref without oci prefix", "imageRefWithoutOciPrefix", imageRefWithoutOciPrefix) | ||
| // Parse the image reference (e.g., docker.io/library/alpine:latest) |
There was a problem hiding this comment.
| i.log.Info("log image ref without oci prefix", "imageRefWithoutOciPrefix", imageRefWithoutOciPrefix) | |
| // Parse the image reference (e.g., docker.io/library/alpine:latest) |
I think the comment and the log message aren't needed here.
There was a problem hiding this comment.
This is just for testing in the mini-lab. I'd rather keep it until this works but you're right, we don't need in the end.
| err = img.NewImage(h.log).Burn(h.chrootPrefix, image, h.osImageDestination) | ||
| if err != nil { | ||
| return nil, err | ||
| h.log.Info("log oci config", "ociConfig", ociConfig) |
There was a problem hiding this comment.
This is also just for testing in the mini-lab. I'll add a TODO comment two both log statements in order to remove it before merging.
| }) | ||
| } | ||
|
|
||
| // TODO: consider character and block devices -> skipped for now due to root permissions |
There was a problem hiding this comment.
I agree that a potential afero refactoring is out of scope for this PR. But I think it would be nice to do (I'm not a maintainer though).
These (and the above FIXME and similar cases) would be really important.
There was a problem hiding this comment.
@majst01 what are your thoughts about this?
4 participants
Description
References
TODO