-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Add contributors guide #9062
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
Add contributors guide #9062
Conversation
mikebrow
left a comment
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.
should probably resolve this against the https://github.com/containerd/containerd#getting-started path so we don't have two different entry points.. one that skips over this one, let's add the link to this contributors guide there ^
laurazard
left a comment
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.
LGTM, left a nit re: file name agreeing with what @AkihiroSuda said
04387ec to
0e66697
Compare
|
Updated @mikebrow yeah our getting started flow and website makes me cringe a little bit. I added a link, but we really need some help setting up a more unified experience for getting started. |
@dmcgowan This doesnt include splitting the api package right? The one which we planned to do via automatic syncing. |
Signed-off-by: Derek McGowan <derek@mcg.dev>
Replace link to BUILDING since CONTRIBUTING already points to it Signed-off-by: Derek McGowan <derek@mcg.dev>
0e66697 to
1a7490c
Compare
Oh, just asking; was your intent with those examples to put packages in a
The "use a subdirectory" approach was also for compatibility with old dependency tools. When moving to It's possible to associate them, but in that case it would require aliases to be provided, so v1 must depend on v2, and make the v1 types aliases for v2. package foo // this is "foo" in the v1 module
// Import the v2 module, which will be code that's vendored
// so the vendor directory will have a copy of containerd from
// a v2 tag.
//
// go.mod will have something like:
//
// require github.com/containerd/containerd/v2 v2.0.0-alpha.0
import "github.com/containerd/containerd/v2/foo"
// Hello is the v1 alias for v2.
type Hello = foo.HelloI created a quick example repository with that situation; look at the commit (and tag) history to get a bit of an idea what the flow was for that. Perhaps there's other options possible; the biggest challenge is how to match signatures. The alternative could be to have v1 -> v2 adaptor code live in the v2 version (to type-cast v1 types to v2) |
|
And of course I didn't post a link to my example repository; here it is; https://github.com/thaJeztah/mod-migrate |
Was the approach I was thinking based on the previous attempt to rename the module. We already have release branches for maintaining the "v1" module. My understanding is there is no clean way to avoid "/v2" in our import paths once we create the "v2.0.0" tag. |
|
@dcantah Cleaned up the BUILDING and CONTRIBUTING links I think we can move the v2 module discussion to that PR for now. We know we need it but still figuring out the how. |
|
Ah, yes, sorry for somewhat derailing this PR (and we should / can definitely pick up that discussion elsewhere). I thought for a bit that the intent of that comment was "going forward, create a @dmcgowan and I caught up a bit on Slack, and discussed some of the issues at hand (lots of further brainstorming will still be needed though!) |
thaJeztah
left a comment
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.
LGTM
fuweid
left a comment
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.
LGTM
mikebrow
left a comment
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.
LGTM
laurazard
left a comment
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.
LGTM
The "Where to put packages" section at the end is proposing the layout for packages for the containerd 2.0 release. While this may seem like a large breaking change, it will align with the go module
/v2change we already need to make.Example package renames required by importers of v2
github.com/containerd/containerd->github.com/containerd/containerd/v2/clientgithub.com/containerd/containerd/archive->github.com/containerd/containerd/v2/pkg/archivegithub.com/containerd/containerd/sys->github.com/containerd/containerd/v2/pkg/sysActual changes to move packages and/or create new go submodules will be in follow up. This is also an opportunity to split out some packages into their own repository when it makes sense (this has been proposed for
log).