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

feat: add env driver #909

Closed
wants to merge 1 commit into from
Closed

Conversation

developer-guy
Copy link
Contributor

@developer-guy developer-guy commented Jan 9, 2022

Signed-off-by: Batuhan Apaydın [email protected]

Fixes #23

cc: @AkihiroSuda @tonistiigi

@AkihiroSuda AkihiroSuda marked this pull request as draft January 9, 2022 12:46
@errordeveloper errordeveloper self-assigned this Jan 10, 2022
@errordeveloper errordeveloper added the kind/enhancement New feature or request label Jan 10, 2022
commands/util.go Outdated Show resolved Hide resolved
driver/manager.go Outdated Show resolved Hide resolved
commands/create.go Outdated Show resolved Hide resolved
commands/util.go Outdated Show resolved Hide resolved
driver/manager.go Outdated Show resolved Hide resolved
commands/create.go Outdated Show resolved Hide resolved
driver/remote/driver.go Outdated Show resolved Hide resolved
@developer-guy developer-guy changed the title WIP: feat: add remote driver feat: add remote driver Mar 17, 2022
@@ -80,6 +81,13 @@ func driversForNodeGroup(ctx context.Context, dockerCli command.Cli, ng *store.N
defer func() {
dis[i] = di
}()

buildkitAPI, err := buildkitclient.New(ctx, n.Endpoint)
Copy link
Contributor

@Dentrax Dentrax Mar 17, 2022

Choose a reason for hiding this comment

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

i think we're initializing both buildkitAPI and dockerapi here if we set --driver=env. So dockerapi initializing unnecessarily. But couldn't find a better way. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

This is definitely not allowed. Only the driver.Client() is allowed to call this. dockerapi initialization is for the features like --load that move things to Docker.

Another difference is that clientForEndpoint is just a struct setup, while buildkitclient.New() actually dials out to the endpoint.

@developer-guy
Copy link
Contributor Author

developer-guy commented Mar 17, 2022

We've tested it by providing a buildkitd.sock through our buildkit-machine project, everything worked fine, right now. Thanks a ton, @Dentrax.

$ buildkit-machine start buildkitd --unix /tmp/buildkitd.sock
$ docker buildx create --name demo --driver env /tmp/buildkit.sock --use

# build some random go project
$ docker buildx build -t <tag> <img> .

@developer-guy
Copy link
Contributor Author

Screen Shot 2022-03-17 at 20 29 48
Screen Shot 2022-03-17 at 20 29 28

@developer-guy
Copy link
Contributor Author

cc: @crazy-max

@developer-guy developer-guy marked this pull request as ready for review March 17, 2022 17:41
@developer-guy developer-guy changed the title feat: add remote driver feat: add env driver Mar 17, 2022
@developer-guy developer-guy force-pushed the master branch 2 times, most recently from f1bf153 to 9a816c3 Compare March 17, 2022 19:42
Signed-off-by: Batuhan Apaydın <[email protected]>
Co-authored-by: Furkan Türkal <[email protected]>
Signed-off-by: Batuhan Apaydın <[email protected]>
}

func (d *Driver) Features() map[driver.Feature]bool {
return map[driver.Feature]bool{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why empty?

@tonistiigi
Copy link
Member

@developer-guy Any update on the comments? Lmk if you have questions.

@jedevc jedevc mentioned this pull request Apr 25, 2022
@developer-guy
Copy link
Contributor Author

@developer-guy Any update on the comments? Lmk if you have questions.

Hi @tonistiigi, sorry for delay, I was in military, I've just returned, I'll continue where I left, thanks for the mention 🙋🏻‍♂️

@jedevc
Copy link
Collaborator

jedevc commented Apr 28, 2022

@developer-guy have you seen #1078? I've tried to finish off the work you started, do let me know what you think 😄

@tonistiigi
Copy link
Member

Merged in #1078

@tonistiigi tonistiigi closed this Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add "env" driver that connects to $BUILDKIT_HOST
6 participants