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

ericstoekl · 2017-09-08T15:50:31Z

Create faas-cli add-template --url <URL> command
Add template.yml files for each language in the ./template dir.
Read fProcess from ./template/ in deploy command, instead of using hard-coded value
Adds mock-server test

Description

Adds the addTemplate command and a basic test for it. Changes the way that fProcess command is acquired: from a hard-coded value in proxy/deploy.go to a new YAML file in each language-specific dir in the template/ dir, for example template/csharp/template.yml.

Motivation and Context

I have raised an issue to propose this change (required)
Proposal: third-party templates via GitHub #85

How Has This Been Tested?

Tests added in addTemplate_test.go

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 · 2017-09-08T16:00:16Z

Looking forward to looking at this. One quick note. It would be "add-template" for the command and add_template for filenames.

…

On 8 Sep 2017 16:50, "Erich Stoekl" ***@***.***> wrote: 1. Create faas-cli addTemplate --URL <URL> command 2. Add template.yml files for each language in the template/ dir. 3. Read fProcess from template in deploy command, if language of function is not Dockerfile 4. Adds mock-server test for addTemplate.go Description Adds the addTemplate command and a basic test for it. Changes the way that fProcess command is acquired: from a hard-coded value in proxy/deploy.go to a new YAML file in each language-specific dir in the template/ dir, for example template/csharp/template.yml. Motivation and Context - I have raised an issue to propose this change (required <https://github.com/alexellis/faas/blob/master/CONTRIBUTING.md>) #85 <#85> How Has This Been Tested? Tests added in addTemplate_test.go 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 <https://github.com/alexellis/faas/blob/master/CONTRIBUTING.md> 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. ------------------------------ You can view, comment on, or merge this pull request online at: #87 Commit Summary - 1. Create `faas-cli addTemplate --URL <URL>` command File Changes - *A* commands/addTemplate.go <https://github.com/alexellis/faas-cli/pull/87/files#diff-0> (52) - *A* commands/addTemplate_test.go <https://github.com/alexellis/faas-cli/pull/87/files#diff-1> (54) - *M* commands/deploy.go <https://github.com/alexellis/faas-cli/pull/87/files#diff-2> (23) - *M* commands/list.go <https://github.com/alexellis/faas-cli/pull/87/files#diff-3> (1) - *M* proxy/deploy.go <https://github.com/alexellis/faas-cli/pull/87/files#diff-4> (10) - *A* stack/language_template.go <https://github.com/alexellis/faas-cli/pull/87/files#diff-5> (31) - *M* stack/schema.go <https://github.com/alexellis/faas-cli/pull/87/files#diff-6> (5) - *A* template/csharp/template.yml <https://github.com/alexellis/faas-cli/pull/87/files#diff-7> (2) - *A* template/node-armhf/template.yml <https://github.com/alexellis/faas-cli/pull/87/files#diff-8> (2) - *A* template/node/template.yml <https://github.com/alexellis/faas-cli/pull/87/files#diff-9> (2) - *A* template/python-armhf/template.yml <https://github.com/alexellis/faas-cli/pull/87/files#diff-10> (2) - *A* template/python/template.yml <https://github.com/alexellis/faas-cli/pull/87/files#diff-11> (2) - *A* template/ruby/template.yml <https://github.com/alexellis/faas-cli/pull/87/files#diff-12> (2) Patch Links: - https://github.com/alexellis/faas-cli/pull/87.patch - https://github.com/alexellis/faas-cli/pull/87.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#87>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGEGzze92XaFzkhEpFD-cqV8HsNlkHotks5sgWJIgaJpZM4PRXcH> .

ericstoekl · 2017-09-09T17:49:09Z

Updated file names, improved help for add-templates command

alexellis · 2017-09-11T03:09:05Z

For reference let's update the description of this PR with the naming "add-template".

alexellis · 2017-09-11T03:11:21Z

commands/add_template.go

+	URL = strings.TrimRight(URL, "/")
+	URL = URL + "/archive/master.zip"
+
+	err := os.Setenv("templateUrl", URL)


Is there a way to avoid passing the URL via environmental variable?

Sure. I can change fetchTemplates() in fetch_temlates.go to accept a param of the custom URL.

The updated version no longer uses env var.

ericstoekl · 2017-09-13T18:22:35Z

Question about this PR: should I add some logic to scan through the repository that is passed in to search for the template dir? Or should we look for third party templates in the root path of a repo? Currently fetchTemplates() only looks for the template dir in the faas-cli-master/ directory, because as the code is written right now, it only expects to be grabbing a template dir from the alexellis/faas-cli repo. The implication of this is that the repository containing 3rd party templates needs to be named faas-cli. We could have a 'custom URL' flag that fetchTemplates() takes to tell that the repo may be named anything. Or maybe if a custom URL is passed into fetchTemplates(), it just uses whatever repo name the URL points to.

https://github.com/alexellis/faas-cli/blob/3a2e81471c69275571d71f84c158bb787fb1ad7b/commands/fetch_templates.go#L31

ghost · 2017-09-17T22:53: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-18T00:38:08Z

Thanks to @itscaro for huge help with this update. This update now supercedes #66. In this change the templateUrl environment variable usage is no longer possible.

alexellis · 2017-09-20T16:00:09Z

@ericstoekl what's remaining to get this in?

itscaro · 2017-09-20T17:49:39Z

@alexellis It's ready to be merge ;-)

ericstoekl · 2017-09-20T18:31:20Z

@alexellis , I agree with Minh-Quan, it looks ready for merging.

alexellis · 2017-09-23T09:55:46Z

This branch has conflicts that must be resolved

ghost · 2017-09-23T10:17:04Z

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.

itscaro · 2017-09-23T11:34:14Z

@alexellis Conflicts are resolved 👍

alexellis · 2017-10-25T19:08:15Z

commands/fetch_templates.go


+	templateURLSplit := strings.Split(templateURL, "/")
+	templateURLSplit = templateURLSplit[len(templateURLSplit)-4 : len(templateURLSplit)-2]


I don't like the magic numbers -4 -2

What's this doing? maybe break out a function and document the indexes

This is just to name the master.zip file as the name of the repository. It's not necessary so I'm removing it.

alexellis · 2017-10-25T19:09:43Z

commands/fetch_templates.go

@@ -137,3 +184,34 @@ func createPath(relativePath string, perms os.FileMode) error {
 	err := os.MkdirAll(dir, perms)
 	return err
 }
+
+// canWriteLanguage tells whether the language can be processed or not
+// if overwrite is activated, the directory template/language/ is removed before to keep it in sync


@rgee0 @johnmccabe I don't know if overwrite should delete anything. It should probably dump over the top. Not sure?

Delete is a strong action. I do think it important that the user knows exactly what the current state of their templates is and that they aren't able to 'accidentally' overwrite.

I have updated it to no longer delete before --overwrite.

alexellis · 2017-10-25T19:10:23Z

commands/fetch_templates.go

+// if overwrite is activated, the directory template/language/ is removed before to keep it in sync
+func canWriteLanguage(language string, overwrite bool) bool {
+
+	if len(language) > 0 {


I'm struggling to read/decipher this function - think it can be made simpler?

alexellis · 2017-10-25T19:10:53Z

commands/invoke.go

@@ -51,7 +51,7 @@ func runInvoke(cmd *cobra.Command, args []string) {
 	functionName = args[0]

 	if len(yamlFile) > 0 {
-		parsedServices, err := stack.ParseYAMLFile(yamlFile, regex, filter)
+		parsedServices, err := stack.ParseYAMLFileForStack(yamlFile, regex, filter)


Why ParseYAMLFileForStack vs ParseYAMLFile?

Changed this one back.

alexellis · 2017-10-25T19:11:27Z

commands/new_function.go

@@ -45,14 +45,20 @@ language or type in --list for a list of languages available.`,

 func runNewFunction(cmd *cobra.Command, args []string) {
 	if list == true {
+		var available_templates string
+
+		if f, err := ioutil.ReadDir(templateDirectory); err != nil {


can we change f to something else please?

alexellis · 2017-10-25T19:12:15Z

commands/new_function.go

+		var available_templates string
+
+		if f, err := ioutil.ReadDir(templateDirectory); err != nil {
+			available_templates = "There is no available languages"


"No language templates were found. Please run 'faas-cli template pull'."

alexellis · 2017-10-25T19:12:52Z

commands/new_function.go

+		} else {
+			for _, file := range f {
+				if file.IsDir() {
+					available_templates += fmt.Sprintf("- %s\n", file.Name())


Build up an array in a separate function then return this - and have the formatting/printing separate.

alexellis · 2017-10-25T19:13:57Z

commands/new_function_test.go


+	if err := os.Chdir("../.."); err != nil {


I don't think I like this because it's cryptic. Please can you explain what's happening?

We need to execute these tests in faas-cli/commands/testdir/new_function directory, so we need to cd into it for the test to run, then cd back to the home directory afterwards (because we don't know what order the tests will execute in).

I've changed this to save the full directory path at the beginning of the function, then cd back into it instead of using the relative ../...

alexellis · 2017-10-25T19:19:36Z

proxy/deploy.go

-	} else if language == "python3" {
-		fprocessTemplate = "python3 index.py"
+	} else {
+		fmt.Printf("Command to be invoked for function %s not found.\n", functionName)


When would this error fire off? There is no hard need to provide an fprocess at the deployment time. Some Dockerfiles will bake this in.

reverting this

alexellis · 2017-10-25T19:20:42Z

stack/common.go

+)
+
+func ParseYAML(
+	file string,


This doesn't read very well split over so many lines. How does it look on one line?

If there is a function being passed in - perhaps extract it to an interface?

What do you mean by extract to an interface?

Actually removing this entire file for now

alexellis · 2017-10-25T19:20:57Z

stack/common.go

+	file string,
+	parseFunc func([]byte, ...string) (interface{}, error),
+	parseFuncArgs ...string,
+) (interface{}, error) {


Why does this return interface{}?

I think the interface may not be necessary for what we're doing here, I might refactor it to something more simple

It was for mutualise this function for parsing Stack & Language

We will add this back in the next re-factoring PR

alexellis · 2017-10-25T19:21:39Z

stack/language_template.go

+
+// iParseYAMLDataForLanguageTemplate parses YAML data into language template
+// Use the alias ParseYAMLDataForLanguageTemplate
+func iParseYAMLDataForLanguageTemplate(fileData []byte, args ...string) (interface{}, error) {


What's the "i"-prefix convention?

Because it returns an interface.

Actually I'm going to try doing this without the interface approach.

Actually "i" was for "internal", I wanted not to expose it and use the public method to do the assertion on return type.

That's why ParseYAMLDataForLanguageTemplate calls iParseYAMLDataForLanguageTemplate.

This re-factoring is because I wanted to extract ParseYAMLFile to ParseYAML, since the part of parsing can be re-used for both LanguageTemplate & Stack

I am thinking we will add this re-factoring in a future PR, after this one is merged. The plan for this PR is to add minimal functionality.

alexellis · 2017-10-25T19:22:35Z

stack/stack.go

-		if err != nil {
-			return nil, err
-		}
+		return object.(*Services), nil


I'm not sure why we need to use untyped interfaces?

alexellis

Eric I think this will be a good addition to the CLI - it's been requested a number of times.

This PR touches a lot of files (at least 35) and involves both refactoring and new features. I have added some notes and questions.

i would like us to resolve as much as possible before merging.

itscaro · 2017-10-28T10:12:27Z

@ericstoekl I add a verif when downloading an invalid url, the zip is removed if it's not valid.
The command prints out now the list of fetched languages, same as when it does not overwrite
I updated the guide too

ericstoekl · 2017-10-29T06:19:25Z

Squashed the commits into 2 separate ones - First commit is everything before Alex's comments on 10/25, second commit is the changes to address those comments.

After the latest commit, the functionality is all the same, but the code has been changed in many places for simplification.

1. add-template updates 2. overwrite functionality 3. cacheLanguages for faster parsing, cache directory for zip file 4. tests for add-template, with overwrite option 5. test for PullTemplates function 1) Enable github repo ending in '/' 2) Allow 'faas-cli add-template' (no need to specify repo), picks default repo 3) More concise (shorter) summary of language templates applied/not applied 4) Change validTemplate to public facing IsValidTemplate function in stack package 5) Refactor BuildImage() function to use IsValidTemplate to check if language is valid (rather than hard-coded values) 6) Remove --lang option from invoke command (it is not used) Signed-off-by: Eric Stoekl <ems5311@gmail.com> Signed-off-by: Minh-Quan TRAN <account@itscaro.me>

2) Update guide with new links 3) Change to simplified version of ParseYAMLFile/ParseYAMLFileForLanguageTemplate 4) simplify canWriteLanguage map, remove global variable usage 5) Don't print error message if no fprocess supplied (proxy/deploy.go) 6) Use absolute filepath for switching back to home directory after each new_function_test 7) Don't remove files before --overwrite Signed-off-by: Eric Stoekl <ems5311@gmail.com> Signed-off-by: Minh-Quan TRAN <account@itscaro.me>

burtonr · 2017-11-03T02:11:36Z

Ran the tests on my machine and they all passed.
Pulled @ericstoekl Bash template from https://github.com/ericstoekl/faas-custom-templates and made a simple bash function.

Tried the command without parameters and found it pulls the templates from this repo's templates. Something I didn't expect, but it didn't overwrite changes to the existing templates, so I don't see a problem with that.

Nice work!

ericstoekl · 2017-11-03T05:18:07Z

Work in this PR is continued here: #201

ericstoekl force-pushed the addTemplateCmd branch from eb76a4f to 9543c0e Compare September 9, 2017 17:46

ericstoekl force-pushed the addTemplateCmd branch from 9543c0e to 5bc737c Compare September 10, 2017 01:23

ericstoekl mentioned this pull request Sep 11, 2017

Proposal: third-party templates via GitHub #85

Closed

ericstoekl changed the title ~~Create faas-cli addTemplate --URL <URL> command~~ Create faas-cli add-template --url <URL> command Sep 11, 2017

alexellis requested changes Sep 13, 2017

View reviewed changes

ericstoekl mentioned this pull request Sep 15, 2017

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

Merged

11 tasks

ghost added the no-dco label Sep 17, 2017

ericstoekl force-pushed the addTemplateCmd branch from c91e809 to 8ff3fdc Compare September 18, 2017 00:23

ghost removed the no-dco label Sep 18, 2017

ericstoekl force-pushed the addTemplateCmd branch from 8ff3fdc to cd9bb4c Compare September 18, 2017 00:27

itscaro force-pushed the addTemplateCmd branch from a5bac0e to cd9bb4c Compare September 23, 2017 10:11

ghost added the no-dco label Sep 23, 2017

itscaro force-pushed the addTemplateCmd branch 4 times, most recently from 12b7ddb to a6ebcc9 Compare September 23, 2017 11:20

ghost removed the no-dco label Sep 23, 2017

itscaro force-pushed the addTemplateCmd branch from a6ebcc9 to 163eec4 Compare September 23, 2017 11:36

ericstoekl force-pushed the addTemplateCmd branch from 1f9c06d to 37c4fef Compare October 24, 2017 05:31

alexellis reviewed Oct 25, 2017

View reviewed changes

alexellis requested changes Oct 25, 2017

View reviewed changes

ericstoekl force-pushed the addTemplateCmd branch from 37c4fef to 4b13408 Compare October 29, 2017 06:08

ericstoekl closed this Oct 29, 2017

ericstoekl reopened this Oct 29, 2017

ericstoekl force-pushed the addTemplateCmd branch from 4b13408 to 15b9aa7 Compare October 29, 2017 06:36

ericstoekl added 2 commits October 31, 2017 20:07

ericstoekl force-pushed the addTemplateCmd branch from 15b9aa7 to 43b9a12 Compare November 1, 2017 03:09

ericstoekl mentioned this pull request Nov 3, 2017

Adds 'faas-cli template pull' command #201

Closed

11 tasks

ericstoekl closed this Nov 3, 2017


		templateURLSplit := strings.Split(templateURL, "/")
		templateURLSplit = templateURLSplit[len(templateURLSplit)-4 : len(templateURLSplit)-2]

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

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

Conversation

ericstoekl commented Sep 8, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

alexellis commented Sep 8, 2017 via email

ericstoekl commented Sep 9, 2017

alexellis commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstoekl commented Sep 13, 2017

ghost commented Sep 17, 2017

ericstoekl commented Sep 18, 2017 • edited Loading

alexellis commented Sep 20, 2017

itscaro commented Sep 20, 2017

ericstoekl commented Sep 20, 2017

alexellis commented Sep 23, 2017

ghost commented Sep 23, 2017

itscaro commented Sep 23, 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

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

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

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

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

alexellis left a comment

Choose a reason for hiding this comment

itscaro commented Oct 28, 2017

ericstoekl commented Oct 29, 2017

burtonr commented Nov 3, 2017

ericstoekl commented Nov 3, 2017

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

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

ericstoekl commented Sep 8, 2017 •

edited

Loading

ericstoekl commented Sep 18, 2017 •

edited

Loading