Added squash flag & associated fixes #24
Conversation
Can you be specific about this? |
app.go
Outdated
|
|
||
| switch language { | ||
| case "node", "python": | ||
| tempPath := createBuildTemplate(functionName, handler, language) | ||
|
|
||
| fmt.Printf("Building: %s with Docker. Please wait..\n", image) | ||
|
|
||
| cacheFlag := "" | ||
| //lets create a buffer for any build flags | ||
| var flagBuf bytes.Buffer |
There was a problem hiding this comment.
Could this be a string instead?
There was a problem hiding this comment.
It could. What I've read though suggests that using byte buffer is better performance wise when there is any kind of variability in length - strings are immutable. Less of a consideration with 3 functions and four concats per function, granted, but I'd hope users would be using the CLI for batching many more functions.
There was a problem hiding this comment.
I thought that would be the thinking. This code is building a very short string so I think I prefer the string concatenation and readability.
There was a problem hiding this comment.
We should probably extract a method to generate this string (whether it uses a buffer internally or a string) so it can be unit tested.
|
Squashing looks good - just a couple of questions/comments. |
|
Updated the original main change section with further detail about the |
app.go
Outdated
| if len(os.Getenv("http_proxy")) > 0 || len(os.Getenv("http_proxy")) > 0 { | ||
| builder = strings.Split(fmt.Sprintf("docker build %s--build-arg http_proxy=%s --build-arg https_proxy=%s -t %s .", cacheFlag, os.Getenv("http_proxy"), os.Getenv("https_proxy"), image), " ") | ||
| if len(os.Getenv("http_proxy")) > 0 || len(os.Getenv("https_proxy")) > 0 { | ||
| flagBuf.WriteString("--build-arg http_proxy=" + os.Getenv("http_proxy") + " ") |
There was a problem hiding this comment.
Should this be changed to sprintf since we are concatenating strings here and in other places use fmt.Sprintf? Maybe we should also do separate checks to output http_proxy / https_proxy separately
There was a problem hiding this comment.
Well, ultimately, yes, now that we have to go back to strings.
Yes, we could split out the checks, now that they are checking different things.
app.go
Outdated
| } | ||
|
|
||
| builder := strings.Split(fmt.Sprintf("docker build %s-t %s .", flagBuf.String(), image), " ") | ||
| fmt.Println(strings.Join(builder, " ")) |
There was a problem hiding this comment.
Whats the idea here? Seems to split on spaces then join with spaces?
|
Made changes as discussed, committed, squashed and pushed. |
Signed-off-by: rgee0 <richard@technologee.co.uk>
|
Looks very clean and concise. Merged. |
Signed-off-by: rgee0 richard@technologee.co.uk
Main Change:
Added a -squash flag to address #23.
Fixed issue where supplying -no-cache flag would cause builds to fail
Changed http_proxy test to include a test for https_proxy (typo in the OR)
Refactored the generation of the build string to alleviate need for multiple operations in a proxy environment.... Previously the
buildervar would be initialised once outside the proxy test and then reassigned within the proxy test with additional params. By buffering the docker build flags throughout the flag checks we can then just initialisebuilderonce post-checks with code as succinct as the original initialisation.EDIT Added a function
buildFlagStringto build the build flags string, as suggested.Test:
Pre:
On Master ran
./faas-cli -action build -yaml ./samples.yml -no-cacheand confirmed bug:
docker build --no-cache-t alexellis2/faas-urlping .unknown flag: --no-cache-tPost:
Ran
./faas-cli -action build -yaml ./samples.ymland confirmed the command run was
docker build -t alexellis2/faas-urlping .Ran
./faas-cli -action build -yaml ./samples.yml -no-cacheand confirmed the command run was
docker build --no-cache -t alexellis2/faas-urlping .Ran
./faas-cli -action build -yaml ./samples.yml -squashand confirmed the command run was
docker build --squash -t alexellis2/faas-urlping .Ran
./faas-cli -action build -yaml ./samples.yml -no-cache -squashand confirmed the command run was
docker build --no-cache --squash -t alexellis2/faas-urlping .Also tested
-flag=true&-flag=falseand confirmed expected outcomes.Misc:
Used git commit -s as per the contribution guide