Skip to content

Load GitHub runtime environment when using buildctl - #2707

Merged
tonistiigi merged 1 commit into
moby:masterfrom
TomChv:doc/explicit-github-cache-attributes
Mar 15, 2022
Merged

Load GitHub runtime environment when using buildctl#2707
tonistiigi merged 1 commit into
moby:masterfrom
TomChv:doc/explicit-github-cache-attributes

Conversation

@TomChv

@TomChv TomChv commented Mar 4, 2022

Copy link
Copy Markdown
Contributor

When using buildctl, it's not explained in the documentation that GitHub attributes url and token must be manually set to make this works.
That miscomprehension is visible with #2706.

To do not meet that error again, this commit add a precision on how use GitHub cache with buildctl.

Signed-off-by: Vasek - Tom C tom.chauveau@epitech.eu

@TomChv
TomChv force-pushed the doc/explicit-github-cache-attributes branch from 65342ab to d622a11 Compare March 4, 2022 17:24
@tonistiigi

tonistiigi commented Mar 4, 2022

Copy link
Copy Markdown
Member

We could also just add support for this in buildctl for consistency.

@TomChv

TomChv commented Mar 4, 2022

Copy link
Copy Markdown
Contributor Author

I agree, if you leave me time, I can take care of it.
Should I just add a if statement in parseCacheOptions to inject ACTIONS_CACHE_URL and ACTIONS_RUNTIME_TOKEN values?

How that should be handle when used with SolveRequest? Should I also implement a parseCacheOption function to take care of that?
I could also definitely fix #2599 at the same time with that change : #2599 (comment)

@tonistiigi

tonistiigi commented Mar 4, 2022

Copy link
Copy Markdown
Member

It should be done in buildctl pkg not in the client

buildkit/cmd/buildctl/build/importcache.go

Line 43 in 0a7d890

func ParseImportCache(importCaches []string) ([]client.CacheOptionsEntry, error) {

@TomChv

TomChv commented Mar 4, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Okay!

Then I have a question : if someone directly the client, will it works? Or it's the responsibility of the user to inject values?

@tonistiigi

tonistiigi commented Mar 4, 2022

Copy link
Copy Markdown
Member

If you use client pkg directly then this should be taken care by the caller. Client is not a singleton and should not be taking inputs from the running environment.

@TomChv
TomChv force-pushed the doc/explicit-github-cache-attributes branch from f29ac5b to 4c4188e Compare March 8, 2022 20:44
@TomChv

TomChv commented Mar 8, 2022

Copy link
Copy Markdown
Contributor Author

@tonistiigi I updated the PR to supports GitHub runtime environment with buildctl

@tonistiigi

tonistiigi commented Mar 9, 2022

Copy link
Copy Markdown
Member

@TomChv check the CI

tonistiigi
tonistiigi reviewed Mar 9, 2022
View reviewed changes
Comment thread cmd/buildctl/build/exportcache.go Outdated
ex.Attrs["mode"] = "min"
}
if ex.Type == "gha" {
return bindGithubCacheAttributes(ex)

@tonistiigi tonistiigi Mar 9, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

loadGithubEnv

@TomChv TomChv Mar 10, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 6ac606c

@TomChv
TomChv force-pushed the doc/explicit-github-cache-attributes branch from 4c4188e to c9f4124 Compare March 9, 2022 10:58
@TomChv

TomChv commented Mar 9, 2022

Copy link
Copy Markdown
Contributor Author

@TomChv check the CI

My bad, I forget the push the most important haha.
It should be better now!

@TomChv

TomChv commented Mar 9, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Looks like t.Setenv do not exist on windows because you build with go1.16 and t.Setenv has been added in go1.17.
Should I replace t.Setenv to os.Setenv? Or you plan to upgrade go version?

I'm waiting for your response before doing any changes @tonistiigi

@tonistiigi

tonistiigi commented Mar 9, 2022

Copy link
Copy Markdown
Member

Looks like the go version for windows was not updated. https://github.com/moby/buildkit/blob/master/.github/workflows/build.yml#L143 That's the only part of CI that is not in containers. Just update it to 1.17 . Do this in a separate commit (or PR).

tonistiigi
tonistiigi reviewed Mar 9, 2022
View reviewed changes
Comment thread cmd/buildctl/build/util.go Outdated
package build

import (
"errors"

@tonistiigi tonistiigi Mar 9, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

pkg/errors

@TomChv TomChv Mar 10, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 6ac606c

@TomChv

TomChv commented Mar 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Looks like the go version for windows was not updated. https://github.com/moby/buildkit/blob/master/.github/workflows/build.yml#L143 That's the only part of CI that is not in containers. Just update it to 1.17 . Do this in a separate commit (or PR).

Done in #2719, gonna rebase after getting that one merged

@TomChv
TomChv force-pushed the doc/explicit-github-cache-attributes branch 2 times, most recently from 6ac606c to 2f68ff4 Compare March 11, 2022 13:52
@TomChv TomChv changed the title Improve GitHub cache documentation Load GitHub runtime environment when using buildctl Mar 11, 2022
@TomChv

TomChv commented Mar 11, 2022

Copy link
Copy Markdown
Contributor Author

@tonistiigi Looks like it's ready to be merged :D

@gerhard gerhard mentioned this pull request Mar 14, 2022
Closed
5 tasks
tonistiigi
tonistiigi reviewed Mar 15, 2022
View reviewed changes

@tonistiigi tonistiigi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good but squash your commits as atm your second commit reverts the first one.

@TomChv
Improve buildctl import and export cache to use GitHub env variables
8d823f8 
Signed-off-by: Vasek - Tom C <tom.chauveau@epitech.eu>
@TomChv
TomChv force-pushed the doc/explicit-github-cache-attributes branch from 2f68ff4 to 8d823f8 Compare March 15, 2022 13:46
@TomChv

TomChv commented Mar 15, 2022

Copy link
Copy Markdown
Contributor Author

Done, it's ready to be merged 🚀

@tonistiigi
tonistiigi merged commit 1888aab into moby:master Mar 15, 2022
@tonistiigi tonistiigi mentioned this pull request Apr 4, 2022
Closed
@dhirschfeld

dhirschfeld commented Nov 7, 2022

Copy link
Copy Markdown

It seems this hasn't yet made it into a release?

@dhirschfeld

dhirschfeld commented Jan 16, 2023

Copy link
Copy Markdown

It seems this hasn't yet made it into a release?

Looks like this is in 0.11.0 🎉

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@TomChv @tonistiigi @dhirschfeld