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.

Sign up for GitHub

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

Jump to bottom

Load GitHub runtime environment when using buildctl #2707

Merged
merged 1 commit into from
Mar 15, 2022
Merged

Load GitHub runtime environment when using buildctl #2707

merged 1 commit into from
Mar 15, 2022

Conversation

TomChv
Copy link
Contributor

@TomChv TomChv commented Mar 4, 2022

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
Copy link
Member

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

@TomChv
Copy link
Contributor Author

TomChv commented Mar 4, 2022

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
Copy link
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
Copy link
Contributor Author

TomChv commented Mar 4, 2022 •
edited
Loading

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
Copy link
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
Copy link
Contributor Author

TomChv commented Mar 8, 2022

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

@tonistiigi
Copy link
Member

@TomChv check the CI

tonistiigi
tonistiigi reviewed Mar 9, 2022
View reviewed changes
cmd/buildctl/build/exportcache.go Outdated
@@ -39,6 +39,9 @@ func parseExportCacheCSV(s string) (client.CacheOptionsEntry, error) {
if _, ok := ex.Attrs["mode"]; !ok {
ex.Attrs["mode"] = "min"
}
if ex.Type == "gha" {
return bindGithubCacheAttributes(ex)
Copy link
Member

Choose a reason for hiding this comment

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

loadGithubEnv

Copy link
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
Copy link
Contributor Author

TomChv commented Mar 9, 2022

@TomChv check the CI

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

@TomChv
Copy link
Contributor Author

TomChv commented Mar 9, 2022 •
edited
Loading

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
Copy link
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
cmd/buildctl/build/util.go Outdated
package build

import (
"errors"
Copy link
Member

Choose a reason for hiding this comment

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

pkg/errors

Copy link
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
Copy link
Contributor Author

TomChv commented Mar 10, 2022 •
edited
Loading

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
Copy link
Contributor Author

TomChv commented Mar 11, 2022

@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
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.

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
Copy link
Contributor Author

TomChv commented Mar 15, 2022

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
Copy link

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

@dhirschfeld
Copy link

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