Remove master.zip file after building function with faas-cli build #66

ericstoekl · 2017-09-03T17:51:13Z

This change will remove the master.zip file after the fetchTemplates() function is done creating the templates from the downloaded zip file.

Description

Use the os.Remove() function to remove the zip file, whose name is now stored in a const variable, after making sure that the zip file is still present with os.Stat()

Motivation and Context

I have raised an issue to propose this change (required)
Noticed the zip file hanging around, was confused as to its necessity to remain the directory. However, as long as the templates directory is present in your YAML build file location, the master.zip file will not be used. Therefore, it is not necessary to keep it around.

How Has This Been Tested?

Tried building the node, python, csharp, ruby, and Dockerfile templates, and checked that the master.zip file was removed, but the function worked when deployed.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

alexellis

It looks like the CI build is broken.. @ems5311 please can you take a look?

Would be great if you can squash these commits into one (also)

alexellis · 2017-09-04T06:40:08Z

commands/fetch_templates.go

 	return err
 }

 func fetchMasterZip() error {
 	var err error
 	if _, err = os.Stat("master.zip"); err != nil {
+		err = nil


What's happening there? If there is an error you're ignoring it?

That err variable is being set to nil because after we find that there is an error, we load the master.zip file location from the templateUrl environment variable, or if that env variable does not exist, we grab it from github. The error variable is not needed here, in fact it is a bug if it is not reset to nil, because if you have templateURL env variable set because you intend to load the master.zip from an alternative location, as this test does, then the test will normally fail on "master.zip os.Stat method failed", because that method and set the err at line 67. Let me know if there is a better way. Thanks

ericstoekl · 2017-09-04T15:55:14Z

The magic bytes used in Test_mockBuild() in build_test.go are the data that makes up the smallest possible zip file. Therefore, this will expand without errors when zip.OpenReader() runs on it. Although it will expand into nothing, it will enable the fetch functionality to be tested.

alexellis · 2017-09-04T20:26:16Z

I thought that's what they were - so let's make the code self-documenting and use that variable name.

I still need to look at the os.Stat line as I'm still not clear why we're clearing the error.

Otherwise very excited to see these tests emerging.

ericstoekl · 2017-09-04T22:38:34Z

I was wrong about the os.Stat line. The test works without that change, so I'll revert it.

Also I should be calling PullTemplates(), so the test function/filename should be renamed accordingly.

Edit: Just updated the code with a new commit. Let me know if it looks good, and I'll squash them (wanted to leave unsquashed for now so it's easy to see what was updated)

johnmccabe · 2017-09-05T00:24:58Z

build.sh

@@ -5,3 +5,5 @@ docker build --build-arg http_proxy=$http_proxy --build-arg https_proxy=$https_p
 docker cp faas-cli:/root/faas-cli . && \
 docker rm -f faas-cli

+go test
+go test ./commands


You can replace both commands with a single invocation:

go test ./...

ericstoekl · 2017-09-06T20:29:55Z

Httptest mock server used in this changelist is a solution for issue #45

alexellis · 2017-09-08T17:29:23Z

Once you've had a chance to read feedback and apply if necessary can you squash he commits please?

Thanks for this change. It will mean smaller git ignore files

ericstoekl · 2017-09-09T05:33:51Z

Just squashed into a single commit. The feedback has been addressed.

alexellis · 2017-09-08T17:26:33Z

commands/fetch_templates_test.go

+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+
+		// Write out the minimum number of bytes to make the response a valid .zip file
+		w.Write([]byte{80, 75, 05, 06, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00})


This is a magic variable. Please extract it and give it a name like smallest zip etc

@ems5311 where are we with this?

Sure, will make this change this evening.

alexellis · 2017-09-08T17:27:10Z

commands/fetch_templates_test.go

+	}))
+	defer ts.Close()
+
+	os.Setenv("templateUrl", ts.URL)


Can we avoid using the environmental variable? Since this is now using httptest?

Similar to the 3rd party template issue, I can pass a variable to the PullTemplates() function

alexellis · 2017-09-08T17:28:06Z

commands/fetch_templates.go

-		log.Printf("Writing %dKb to master.zip\n", len(bytesOut)/1024)
-		err = ioutil.WriteFile("./master.zip", bytesOut, 0700)
+		log.Printf("Writing %dKb to %s\n", len(bytesOut)/1024, ZipFileName)
+		err = ioutil.WriteFile(ZipFileName, bytesOut, 0700)
 		if err != nil {
 			log.Println(err.Error())
 		}


Please can you do one final new line print at the end of this function? It will separate the output when someone does new or build for the first time.

ghost · 2017-09-14T07:20:46Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

ericstoekl · 2017-09-14T07:21:47Z

Just pushed this update. I'm gonna need to rebase this if it looks good, but wanted it to be more clear for now.

alexellis · 2017-09-15T16:47:57Z

commands/fetch_templates_test.go

+	"testing"
+)
+
+var SmallestZipFile = []byte{80, 75, 05, 06, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00}


Perfect, this is 10x better. It could start with small s so it's not exported too.

alexellis · 2017-09-15T16:48:42Z

Hi Eric,

With the mantra of "Less is more":

I'm not sure we need keep-archive.. overwrite seems like a good idea. Let's get this finished and then I can merge?

ericstoekl · 2017-09-15T16:54:42Z

keep-archive and overwrite are actually options for the add-template change, which is similar to this change but a completely separate issue. I'll update #87 with those suggestions.

I'll change the SmallestZipFile to smallestZipFile, then squash and we should be good to go with this one.

ericstoekl · 2017-09-17T01:15:21Z

Closing this PR as #87 (with changes pending in my local branch) will supercede this PR. It will provide the same functionality along with the add-template function

alexellis

I thought we were ready to merge this?

ericstoekl · 2017-09-17T15:03:15Z

It's superceded by the add-template changes that Minh and I are working on. It would be too much confusion to merge this, then deal with the merge conflicts with add-template, when the functionality will just be replaced anyway

…

On Sep 17, 2017 2:04 AM, "Alex Ellis" ***@***.***> wrote: ***@***.**** commented on this pull request. I thought we were ready to merge this? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#66 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABah_V7t6A-G2tKvjQ8fWqKzrPsFFXGMks5sjOCAgaJpZM4PLQOM> .

1. Remove master.zip after pulling template directory 2. Tests template fetch with mock HTTP server Signed-off-by: Eric Stoekl <ems5311@gmail.com>

alexellis · 2017-09-22T10:28:59Z

Thank you for re-opening.

ericstoekl mentioned this pull request Sep 4, 2017

Proposal: Mock HTTP client for testing #69

Closed

alexellis reviewed Sep 4, 2017

View reviewed changes

ericstoekl force-pushed the removeMasterZip_signed_fix branch from 6642b37 to 5d00f5f Compare September 4, 2017 15:45

ericstoekl force-pushed the removeMasterZip_signed_fix branch from 5d00f5f to 296f478 Compare September 5, 2017 00:10

johnmccabe reviewed Sep 5, 2017

View reviewed changes

ericstoekl force-pushed the removeMasterZip_signed_fix branch from 38b1f1d to a963f3c Compare September 5, 2017 00:31

ericstoekl force-pushed the removeMasterZip_signed_fix branch from a963f3c to 5e84bf5 Compare September 9, 2017 05:32

ericstoekl force-pushed the removeMasterZip_signed_fix branch from 5e84bf5 to fd5018f Compare September 9, 2017 16:38

alexellis requested changes Sep 13, 2017

View reviewed changes

ghost added the no-dco label Sep 14, 2017

ericstoekl force-pushed the removeMasterZip_signed_fix branch from 267e747 to 26b3452 Compare September 14, 2017 14:51

itscaro mentioned this pull request Sep 14, 2017

Modifications in fetch_Templates + overwrite and keep-archive flags ericstoekl/faas-cli#1

Merged

alexellis reviewed Sep 15, 2017

View reviewed changes

ericstoekl closed this Sep 17, 2017

alexellis reviewed Sep 17, 2017

View reviewed changes

ericstoekl mentioned this pull request Sep 18, 2017

Create faas-cli add-template --url <URL> command #87

Closed

11 tasks

ericstoekl reopened this Sep 22, 2017

ericstoekl force-pushed the removeMasterZip_signed_fix branch from 3d6c6de to ea738f9 Compare September 22, 2017 05:33

ghost removed the no-dco label Sep 22, 2017

Remove master.zip after unpacking templates from archive

cf4777a

1. Remove master.zip after pulling template directory 2. Tests template fetch with mock HTTP server Signed-off-by: Eric Stoekl <ems5311@gmail.com>

ericstoekl force-pushed the removeMasterZip_signed_fix branch from ea738f9 to cf4777a Compare September 22, 2017 06:00

alexellis approved these changes Sep 22, 2017

View reviewed changes

alexellis merged commit db425d5 into openfaas:master Sep 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove master.zip file after building function with faas-cli build #66

Remove master.zip file after building function with faas-cli build #66

ericstoekl commented Sep 3, 2017 •

edited

Loading

alexellis left a comment

alexellis Sep 4, 2017

ericstoekl Sep 4, 2017

ericstoekl commented Sep 4, 2017

alexellis commented Sep 4, 2017

ericstoekl commented Sep 4, 2017 •

edited

Loading

johnmccabe Sep 5, 2017

ericstoekl commented Sep 6, 2017

alexellis commented Sep 8, 2017

ericstoekl commented Sep 9, 2017

alexellis Sep 8, 2017

alexellis Sep 13, 2017

ericstoekl Sep 13, 2017

alexellis Sep 8, 2017

ericstoekl Sep 13, 2017

alexellis Sep 8, 2017

ghost commented Sep 14, 2017

ericstoekl commented Sep 14, 2017

alexellis Sep 15, 2017 •

edited

Loading

alexellis commented Sep 15, 2017

ericstoekl commented Sep 15, 2017

ericstoekl commented Sep 17, 2017 •

edited

Loading

alexellis left a comment

ericstoekl commented Sep 17, 2017 via email

alexellis commented Sep 22, 2017

Remove master.zip file after building function with faas-cli build #66

Remove master.zip file after building function with faas-cli build #66

Conversation

ericstoekl commented Sep 3, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

alexellis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstoekl commented Sep 4, 2017

alexellis commented Sep 4, 2017

ericstoekl commented Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

ericstoekl commented Sep 6, 2017

alexellis commented Sep 8, 2017

ericstoekl commented Sep 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 14, 2017

ericstoekl commented Sep 14, 2017

alexellis Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

alexellis commented Sep 15, 2017

ericstoekl commented Sep 15, 2017

ericstoekl commented Sep 17, 2017 • edited Loading

alexellis left a comment

Choose a reason for hiding this comment

ericstoekl commented Sep 17, 2017 via email

alexellis commented Sep 22, 2017

ericstoekl commented Sep 3, 2017 •

edited

Loading

ericstoekl commented Sep 4, 2017 •

edited

Loading

alexellis Sep 15, 2017 •

edited

Loading

ericstoekl commented Sep 17, 2017 •

edited

Loading