[go: nahoru, domu]

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

remove oci-distribution #695

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

bacongobbler
Copy link
Collaborator

@thomastaylor312
Copy link
Member

Looks like similar build failures to #694

Copy link
Member
@thomastaylor312 thomastaylor312 left a comment

Choose a reason for hiding this comment

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

Good to go once we solve the underlying build failures and rebase on that work

@bacongobbler bacongobbler force-pushed the remove-oci-distribution branch 2 times, most recently from cd20539 to bfe4dc3 Compare October 22, 2021 21:00
new location: https://github.com/krustlet/oci-distribution

Signed-off-by: Matthew Fisher <matt.fisher@fishworks.io>
Signed-off-by: Matthew Fisher <matt.fisher@fermyon.com>
Signed-off-by: Matthew Fisher <matt.fisher@fermyon.com>
@thomastaylor312
Copy link
Member

@bacongobbler I think with a rebase this should be good to go

@bacongobbler
Copy link
Collaborator Author

After re-fetching from upstream:

><> git rebase main
Current branch remove-oci-distribution is up to date.

@thomastaylor312
Copy link
Member

I thought we had fixed the cargo deny stuff recently

@thomastaylor312
Copy link
Member

@bacongobbler Let's go ahead an ignore the cargo deny stuff for now. We should be able to fix that before we release the beta. So this should be good to merge for now

@rickrain
Copy link
rickrain commented Jan 24, 2022

Any thoughts on when this PR can be merged?

I'm working on a custom kubelet implementation and using the kubelet crate. In my cargo.toml, I have a git package dependency to this repo instead of a crates.io package dependency so I can pickup the most recent kubelet code that has not yet formally released (see #707).

In my implementation, I'm also making use of the GenericProvider and therefore leveraging the generic pod state implementations (Registered, ImagePull, etc.) - thank you for those btw! And, because kubelet is still referencing oci-distribution as a git package reference, I had to do the same in my project.

Unfortunately, I ran into the issue regarding manifest lists not being supported yet in oci-distribution, and then found this PR in the new oci-distribution repo that handles this for me. Hopefully that will merge into main soon too.

The ideal scenario would be (in order):

  1. Merge this PR in the oci-distribution repo and generate a release for crates.io.
  2. Update cargo.toml in this PR that we're discussing with the new release/version of oci-distribution.
  3. Merge this PR.
  4. Create a new kubelet release for crates.io. Although, I believe just getting 1-3 would unblock me.

I realize I could provide my own ImagePull state, but would prefer not to since the work has been done.

@thomastaylor312
Copy link
Member

Thanks for reaching out @rickrain! So normally our rule is that maintainers merge their own PRs once reviewed. The other PR you referred to is awaiting review from someone with more OCI experience than myself. So I think we can do all of those things.

I'll start out by pinging @bacongobbler and see if he is ok with me merging this. Then we can do a follow up PR with a version bump as soon as the other feature is added

@thomastaylor312
Copy link
Member

Just heard back from @bacongobbler. Going to go ahead and merge this

@thomastaylor312 thomastaylor312 merged commit 63cca61 into krustlet:main Jan 24, 2022
@bacongobbler
Copy link
Collaborator Author

I'm okay with merging this. Last we revisited this PR, we were waiting on fixing up Windows CI failures, but it looks like that's stalled out. Good to go otherwise

@bacongobbler bacongobbler deleted the remove-oci-distribution branch January 24, 2022 22:56
@rickrain
Copy link

Thanks @thomastaylor312 and @bacongobbler for the quick response and action.

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.

None yet

3 participants