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

Oci support #2566

Closed
wants to merge 26 commits into from
Closed

Oci support #2566

wants to merge 26 commits into from

Conversation

loudej
Copy link
Contributor

@loudej loudej commented Nov 2, 2021

Adds OCI support for kpt pkg get

First PR of a small series to add OCI to an experimental kpt branch

* Adds `oci://` to disambiguate `kpt pkg get` argument
@mikebz
Copy link
Contributor

mikebz commented Nov 3, 2021

Hey @loudej we just launched a public design document template. Could be good to describe how OCI will work in a way that's visible to the user base: https://github.com/GoogleContainerTools/kpt/blob/main/CONTRIBUTING.md

Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @loudej

One thing I was wondering was if we could abstract out the pkg fetcher code such that we can have pkg fetcher implementation for GitPkgFetcher and OciPkgFetcher and we instantiate these as early as possible in the command life-cycle. This could avoid code having switch case for Git and Oci.

WDYT ?

@loudej
Copy link
Contributor Author

loudej commented Nov 3, 2021

Hi @mikebz Sounds like a great idea. As I understand the procedure - this idea is to have an additional PR that contains only a copy of the md and it should be in the same folder as the template?

@droot Sounds good - I'll take a look. I think I know what layer you're talking about (something to use just under the util/*/Command logic)

@mikebz
Copy link
Contributor

mikebz commented Nov 4, 2021

Yes. What I have seen with other products is the same folder but maybe start with a different number. Thank you!

@loudej
Copy link
Contributor Author

loudej commented Nov 5, 2021

@droot Hi again! Well, there was already a fetch Command package and I was concerned fetch and fetcher would be very easy to confused. Instead I added an upstream package because all of the work will be dealing with the upstream sources and upstream/upstreamLock Kptfile properties.

It has anupstream.Fetcher interface that duck types onto upstream.gitUpstream and upstream.ociUpstream. Let me know if that resonates with you or if the names should be shuffled around to something better.

Ah - the impact on the code is that it is cleaner, but it will make it harder to keep the branch in line with main.

Louis DeJardin added 4 commits November 5, 2021 04:42
* this startup check probably doesn't make sense any more? the pkg
update logic doesn't rely on the state of the local git repo to merge
@phanimarupaka
Copy link
Contributor

I am looking forward to the design document, especially highlighting the user guide/journey for get, update and diff commands before I take a stab at reviewing the code.

@loudej
Copy link
Contributor Author

loudej commented Nov 9, 2021

@phanimarupaka sounds great, sorry for the delay! That is my next task on deck, and thanks for the help!

@loudej
Copy link
Contributor Author

loudej commented Nov 16, 2021

Added companion PR #2589 with design doc. Happy to iterate and collaborate on that, hoping it aligns with what you were expecting!

Also PR is smaller scope than design doc - it doesn't support sub-package or diff yet - and push/pull commands aren't added.

/cc @phanimarupaka @droot @mikebz

Louis DeJardin added 2 commits November 18, 2021 07:53
* Mainly involves using upstream.Fetcher methods
Based on design and latest conversations
@loudej
Copy link
Contributor Author

loudej commented Dec 11, 2021

Closing this PR in favor of #2621 which is rebased on current main

@loudej loudej closed this Dec 11, 2021
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.

4 participants