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

llb: update docs #293

Merged
merged 1 commit into from
Feb 27, 2018
Merged

llb: update docs #293

merged 1 commit into from
Feb 27, 2018

Conversation

AkihiroSuda
Copy link
Member

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

@AkihiroSuda AkihiroSuda changed the title llb: move to more easily findable place llb: move to more easily findable place (+ doc updates) Feb 26, 2018
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Why make it top level as nobody really needs to import this package directly. How about client/llb/pb ?

llb/llb.proto Outdated
message SourceOp {
// source type?
// identifier e.g. local://, docker-image://, git://, https://...
Copy link
Member

Choose a reason for hiding this comment

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

keep the comment as I still want to do it. Or make it more readable, "use source type or any type instead of URL protocol"

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

@@ -1,3 +1,3 @@
package moby_buildkit_v1_frontend

//go:generate protoc -I=. -I=../../../vendor/ --gogo_out=plugins=grpc:. gateway.proto
//go:generate protoc -I=. -I=../../../vendor/ -I=../../../../../../ --gogo_out=plugins=grpc:. gateway.proto
Copy link
Member

Choose a reason for hiding this comment

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

follow-up: should do the generation in containers (and validation for CI). Adding more of these includes makes in more dependant on the user environment(eg. symlink in gopath starts to break this)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and I think we should use https://github.com/stevvooe/protobuild

@AkihiroSuda
Copy link
Member Author

Why make it top level as nobody really needs to import this package directly. How about client/llb/pb ?

During conversation at genuinetools/img#18, it appeared that LLB is hard to find for humans.
I think placing the llb directory on the top level is beneficial for helping humans investigate what LLB is.

Also, since LLB is used by both solver and client, I think placing it to client/llb/pb does not seem to solve the problem.

@tonistiigi
Copy link
Member

LLB has big priority in the readme and impossible to miss. It appears as a first section of how to get started.

I don't think the location of the package should be determined like this. In fact, it is bad if people find it before reading the docs, as like I mentioned there are almost no use cases for importing this package unless you want to write a new client in some other programming language.

If you don't like client/llb/pb it can be in solver-next/llb/pb as well(this is where it would have ended up anyway).

@tonistiigi
Copy link
Member

Lets put things in root level that are either base components/modules of controller or meta categories. Eg. group components that are usable on their own. By quick check for the current state, "identity" seems out of place, everything else seemed ok. LLB is a type definition for two subpackages: client/llb and solver-next/llb, so seems very weird to me in the top level.

@AkihiroSuda
Copy link
Member Author

ok, will revert and just keep document part.

Doc part LGTY?

@tonistiigi
Copy link
Member

Doc part LGTY?

Yes, new comments as well.

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@AkihiroSuda AkihiroSuda changed the title llb: move to more easily findable place (+ doc updates) llb: update docs Feb 27, 2018
@AkihiroSuda
Copy link
Member Author

done

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@tonistiigi tonistiigi merged commit 666a5e4 into moby:master Feb 27, 2018
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