-
Notifications
You must be signed in to change notification settings - Fork 10
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
Adds support of buildpackages in buildpack store and updates freezer #302
Conversation
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.
Generally looks good. A couple of non-blocking questions/comments.
packagers/jam.go
Outdated
@@ -47,15 +68,32 @@ func (j Jam) Execute(buildpackDir, output, version string, offline bool) error { | |||
args := []string{ | |||
"pack", | |||
command, filepath.Join(buildpackDir, buildpackOrExtensionToml), | |||
"--output", output, | |||
"--output", filepath.Join(jamOutput, fmt.Sprintf("%s.tgz", version)), |
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.
can we encapsulate fmt.Sprintf("%s.tgz", version)
into a variable - both for re-use but also to give it a useful name
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.
Sure I have no objections. Push something up soon.
pack = &fakes.Executable{} | ||
|
||
tempOutput = func(string, string) (string, error) { | ||
return "some-jam-output", nil |
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.
Does this work if the tests are run in parallel?
I was expecting to see t.TempDir
as a testing substitute for the default os.MkdirTemp
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.
I don't think I have ever seen issues with os.MkdirTemp
and thread safety. It was just the function that I am most familiar with.
Checklist