-
Notifications
You must be signed in to change notification settings - Fork 21
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
Image pull implementation #26
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Yaroslav Bolyukin <[email protected]>
Signed-off-by: Yaroslav Bolyukin <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: CertainLach The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Yaroslav Bolyukin <[email protected]>
e76cea3
to
4b0b935
Compare
Nice, that's wonderful to hear! ❤️
Not yet, but are you interested in contributing to some? :) We could start with a fresh documentation page inside the repository. WDYT? |
Signed-off-by: Yaroslav Bolyukin <[email protected]>
Signed-off-by: Yaroslav Bolyukin <[email protected]>
5fe7fa3
to
6a8e27c
Compare
…rvice Signed-off-by: Yaroslav Bolyukin <[email protected]>
Signed-off-by: Yaroslav Bolyukin <[email protected]>
Signed-off-by: Yaroslav Bolyukin <[email protected]>
…rvice Signed-off-by: Yaroslav Bolyukin <[email protected]>
I reduced scope of this PR, because currently i stuck at some bits of multiple transports support, feel free to merge this as partially finished work, i will commit more transports soon :D |
Can you please rebase and resolve the conflict? 😇 Fixing the CI would be necessary now too. |
@CertainLach: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
Awesome work @CertainLach :) Thanks for your PR. I just had some minor comments, but they are just nits.
Ok(MyImage { | ||
database: sled::open(db_path)?, | ||
layers, | ||
registries: vec![DockerRegistryClientV2::new( |
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 instead of hard coding the registries here maybe in future we can read from the config file, just like how we do it currently in CRI-O.
pub async fn pull_image(&self, spec: &ImageSpec) -> Result<Option<Manifest>> { | ||
// TODO: handle registry name in image name (I.e docker.io/redis:latest) | ||
let parts = spec.image.split(':').collect::<Vec<&str>>(); | ||
assert_eq!(parts.len(), 2, "bad name format"); |
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.
In case image tag is absent, should we just default to latest
instead ?
}; | ||
let mut found_manifest = None; | ||
for manifest in manifests.manifests { | ||
// TODO: check platform with passed annotations |
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 guess you meant in case of index manifest, we should check against architecture and os fields to see if they match.
@CertainLach do you mind giving this a rebase? |
I like the idea of Rust CRI implementation, and want to help in it :D
Is there any implementation plan/architecture description?
If no - i suggest using
sled
for internal storage, this is fast embedded kv database, which is pretty stableWhat type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #16
Special notes for your reviewer:
Does this PR introduce a user-facing change?