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 'faas-cli template pull' command #201

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@ericstoekl
Contributor

ericstoekl commented Nov 3, 2017

Pull language templates from any repo that has a 'template/' directory
in the root with 'faas-cli template pull '.

Also allows 'faas-cli new --list' to list available languages

Signed-off-by: Eric Stoekl ems5311@gmail.com
Signed-off-by: Minh-Quan TRAN account@itscaro.me

Description

This PR is to add the ability to use faas-cli template pull <github repo>. For example:

$ faas-cli template pull https://github.com/itscaro/openfaas-template-php
Fetch templates from repository: https://github.com/itscaro/openfaas-template-php
2017/11/02 21:06:55 HTTP GET https://github.com/itscaro/openfaas-template-php/archive/master.zip
2017/11/02 21:06:56 Writing 8Kb to master.zip

2017/11/02 21:06:56 Attempting to expand templates from master.zip
2017/11/02 21:06:56 Fetched 2 template(s) : [php php5] from https://github.com/itscaro/openfaas-template-php
2017/11/02 21:06:56 Cleaning up zip file...
$ ls template/php5
composer.json  Dockerfile  function  index.php  php-extension.sh  template.yml
$ faas-cli new php_t1 --lang php
Folder: php_t1 created.
  ___                   _____           ____  
 / _ \ _ __   ___ _ __ |  ___|_ _  __ _/ ___| 
| | | | '_ \ / _ \ '_ \| |_ / _` |/ _` \___ \ 
| |_| | |_) |  __/ | | |  _| (_| | (_| |___) |
 \___/| .__/ \___|_| |_|_|  \__,_|\__,_|____/ 
      |_|                                     


Function created in folder: php_t1
Stack file written: php_t1.yml
$ cat php_t1.yml 
provider:
  name: faas
  gateway: http://localhost:8080

functions:
  php_t1:
    lang: php
    handler: ./php_t1
    image: php_t1
$ ls php_t1
composer.json  Handler.php  php-extension.sh

Check this asciicinema to see it in action: https://asciinema.org/a/ieyznKidFlvimpeiS5BFMG3ep

Here's a breakdown of what was changed:

Main Files

  • builder/build.go - Uses IsValidTemplate() to tell whether or not it can build the given language
  • commands/build.go - Update to pass overwrite parameter to fetchTemplates()
  • commands/deploy.go - Get Fprocess from the template/<lang>/template.yml file instead of hard-coded
  • commands/fetch_templates.go - fetchTemplates() method pulls from openfaas/faas-cli github repo by default, custom repo URL can be passed in. Implements --overwrite functionality
  • commands/new_function.go - Lists available languages from the template dir with --list option
  • commands/template_pull.go - Make sure the passed-in valid, then passes it to fetchTemplates()
  • `proxy/deploy.go - Remove hard-coded validation of available language templates
  • stack/language_template.go - Adds ParseYAMLForLanguageTemplate(), which will parse the template/<lang>/template.yml file, which contains info like Fprocess
  • stack/schema.go - Adds LanguageTemplate type
  • guide/TEMPLATE.md - Adds a guide for usage of faas-cli template pull
    • template/csharp/template.yml
    • template/go-armhf/template.yml
    • template/go/template.yml
    • template/node-arm64/template.yml
    • template/node-armhf/template.yml
    • template/node/template.yml
    • template/python-armhf/template.yml
    • template/python/template.yml
    • template/python3/template.yml
    • template/ruby/template.yml

The rest of the files are for testing, and are discussed below.

Motivation and Context

  • I have raised an issue to propose this change (required)
    First discussed in issue #85, originally work was done in #87.

How Has This Been Tested?

In addition to the following files being added for testing, @burtonr reported success when manually testing: #87 (comment)

  • commands/fetch_templates_test.go - Simplify tests, add test for fetchTemplates()
  • commands/new_function_test.go - Test --list option, and add more tests for new
  • commands/template_pull_test.go - Test template pull functionality
  • commands/testdata/master_test.zip - Minimal template zip for testing, served by the mock test server
  • commands/testdata/new_function/template/csharp/function/.gitignore - Add directories
  • commands/testdata/new_function/template/go-armhf/.gitignore
  • commands/testdata/new_function/template/go/function/.gitignore
  • commands/testdata/new_function/template/node-arm64/function/.gitignore
  • commands/testdata/new_function/template/node-armhf/.gitignore
  • commands/testdata/new_function/template/node/function/.gitignore
  • commands/testdata/new_function/template/python-armhf/.gitignore
  • commands/testdata/new_function/template/python/function/.gitignore
  • commands/testdata/new_function/template/python3/function/.gitignore
  • commands/testdata/new_function/template/ruby/function/.gitignore
  • stack/language_template_test.go - Test ParseYAMLForLanguageTemplate() against different types of YAML file

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.

@ericstoekl ericstoekl referenced this pull request Nov 3, 2017

Closed

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

9 of 11 tasks complete
@itscaro

This comment has been minimized.

Show comment
Hide comment
@itscaro

itscaro Nov 5, 2017

Contributor

@ericstoekl new --list instead of new --lang in your PR description


faas-cli template list could be an alias of faas-cli new --list

Contributor

itscaro commented Nov 5, 2017

@ericstoekl new --list instead of new --lang in your PR description


faas-cli template list could be an alias of faas-cli new --list

@alexellis alexellis added this to In review in #TeamServerless Nov 6, 2017

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Nov 6, 2017

Member
$ mkdir -p ./template/bash

$ ./faas-cli new --lang bash abc
Folder: abc created.
2017/11/06 17:33:51 open ./template/bash/function/: no such file or directory

@ericstoekl perhaps we should not make languages available where there is no template.yml file in the directory.

Member

alexellis commented Nov 6, 2017

$ mkdir -p ./template/bash

$ ./faas-cli new --lang bash abc
Folder: abc created.
2017/11/06 17:33:51 open ./template/bash/function/: no such file or directory

@ericstoekl perhaps we should not make languages available where there is no template.yml file in the directory.

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Nov 6, 2017

Member

I've tested the PR and really liked the experience, this will be a great addition to our CLI.

For follow-up work post-merge, let's talk about:

  • How do we shrink-wrap templates?
  • How do we pin a specific commit/SHA or version of a template?
  • What happens at CI time? Do we do faas-cli template pull, or store this data alongside stack.yml or inside $HOME/openfaas.yml?
Member

alexellis commented Nov 6, 2017

I've tested the PR and really liked the experience, this will be a great addition to our CLI.

For follow-up work post-merge, let's talk about:

  • How do we shrink-wrap templates?
  • How do we pin a specific commit/SHA or version of a template?
  • What happens at CI time? Do we do faas-cli template pull, or store this data alongside stack.yml or inside $HOME/openfaas.yml?

@alexellis alexellis requested a review from johnmccabe Nov 6, 2017

Show outdated Hide outdated commands/fetch_templates.go Outdated
Show outdated Hide outdated commands/fetch_templates.go Outdated
Show outdated Hide outdated commands/fetch_templates.go Outdated
@@ -44,15 +44,23 @@ language or type in --list for a list of languages available.`,
}
func runNewFunction(cmd *cobra.Command, args []string) {

This comment has been minimized.

@alexellis

alexellis Nov 6, 2017

Member

We're changing the commands to return (error) so we can handle the exit codes during CI / scripting. If that's beyond scope of the PR please raise an issue instead to log the work.

@alexellis

alexellis Nov 6, 2017

Member

We're changing the commands to return (error) so we can handle the exit codes during CI / scripting. If that's beyond scope of the PR please raise an issue instead to log the work.

This comment has been minimized.

@ericstoekl

ericstoekl Nov 7, 2017

Contributor

I will take you up on the second option: #210

@ericstoekl

ericstoekl Nov 7, 2017

Contributor

I will take you up on the second option: #210

Show outdated Hide outdated commands/new_function.go Outdated
Show outdated Hide outdated commands/template_pull.go Outdated
@@ -0,0 +1,56 @@
# Using template from external repository

This comment has been minimized.

@alexellis

alexellis Nov 6, 2017

Member

I wonder if this should be in the guides in the faas repo?

@alexellis

alexellis Nov 6, 2017

Member

I wonder if this should be in the guides in the faas repo?

Show outdated Hide outdated stack/language_template.go Outdated
Show outdated Hide outdated stack/schema.go Outdated
Show outdated Hide outdated commands/deploy.go Outdated
@alexellis

Very close to finished. Please address comments and licensing then add a new commit, do not squash.

@alexellis alexellis removed the request for review from johnmccabe Nov 6, 2017

ericstoekl added a commit to ericstoekl/faas-cli that referenced this pull request Nov 7, 2017

template pull - address issues from thread openfaas#201
1) Moves code that merges zip file contents into template/ dir into
expandTemplatesFromZip() function. Removes hard-coded constant.
2) Sort list of languages found with 'faas-cli new --list' before
printing
3) Fixes for typos, more descriptive comments, and remove incorrect
license attribution

Signed-off-by: Eric Stoekl <ems5311@gmail.com>
@ericstoekl

This comment has been minimized.

Show comment
Hide comment
@ericstoekl

ericstoekl Nov 7, 2017

Contributor

Hi @alexellis , thanks for your comments. I've attempted to address all of them except the comment about the guide needing to go into the faas repo. Maybe we can have the guide here for now, and move it in a future PR?

I've created a new function in fetch_templates.go called expandTemplatesFromZip(). The goal is to encapsulate the unwieldy blob of code in (the for loop formerly in fetchTemplates()) that was there to unpack the template zip file, and merge it with the existing template/ dir, with the option of overwriting.

I've added sort functionality to the faas-cli new --list command, to sort the list of available language templates before displaying them. This is done by implementing the Len(), Swap(), and Less() functions, as is described by the docs: https://golang.org/pkg/sort/

Contributor

ericstoekl commented Nov 7, 2017

Hi @alexellis , thanks for your comments. I've attempted to address all of them except the comment about the guide needing to go into the faas repo. Maybe we can have the guide here for now, and move it in a future PR?

I've created a new function in fetch_templates.go called expandTemplatesFromZip(). The goal is to encapsulate the unwieldy blob of code in (the for loop formerly in fetchTemplates()) that was there to unpack the template zip file, and merge it with the existing template/ dir, with the option of overwriting.

I've added sort functionality to the faas-cli new --list command, to sort the list of available language templates before displaying them. This is done by implementing the Len(), Swap(), and Less() functions, as is described by the docs: https://golang.org/pkg/sort/

@ericstoekl

This comment has been minimized.

Show comment
Hide comment
@ericstoekl

ericstoekl Nov 7, 2017

Contributor

@alexellis , I have pushed another commit to address this comment: #201 (comment)

This is to disallow creation of a function when template.yml is not found in the template directory.

If we want to split this off into a future PR, I'm okay with that as well.

Contributor

ericstoekl commented Nov 7, 2017

@alexellis , I have pushed another commit to address this comment: #201 (comment)

This is to disallow creation of a function when template.yml is not found in the template directory.

If we want to split this off into a future PR, I'm okay with that as well.

@@ -0,0 +1,2 @@
language: node-armhf
fprocess: node index.js

This comment has been minimized.

@alexellis

alexellis Nov 9, 2017

Member

Does the node template contain a full template or does it still only have the Dockerfile patch? cc @rgee0

@alexellis

alexellis Nov 9, 2017

Member

Does the node template contain a full template or does it still only have the Dockerfile patch? cc @rgee0

Show outdated Hide outdated commands/fetch_templates.go Outdated
Show outdated Hide outdated commands/fetch_templates.go Outdated
Show outdated Hide outdated commands/fetch_templates.go Outdated
Show outdated Hide outdated commands/fetch_templates.go Outdated

ericstoekl added some commits Sep 7, 2017

Adds 'faas-cli template pull' command
Pull language templates from any repo that has a 'template/' directory
in the root with 'faas-cli template pull <github repo>'.

Also allows 'faas-cli new --lang' to list available languages

Signed-off-by: Eric Stoekl <ems5311@gmail.com>
Signed-off-by: Minh-Quan TRAN <account@itscaro.me>
template pull - address issues from thread #201
1) Moves code that merges zip file contents into template/ dir into
expandTemplatesFromZip() function. Removes hard-coded constant.
2) Sort list of languages found with 'faas-cli new --list' before
printing
3) Fixes for typos, more descriptive comments, and remove incorrect
license attribution

Signed-off-by: Eric Stoekl <ems5311@gmail.com>
template pull - disallow function creation when 'template.yml'
is not present in language template directory

Needs to add a 'template.yml' to each fake language template dir in
commands/testdata/new_function/template/<lang> in order to have current
tests pass. Deletes some unnecessary .gitignore files as a result.

Signed-off-by: Eric Stoekl <ems5311@gmail.com>
Updates to 'faas-cli template pull'
1) Improve readability by adding canExpandTemplateData() function, which
returns an enum telling what to do with the data found in the zip
archive.
2) Add function data to ARMHF directories in template/ dir
3) More descriptive naming for test array in language_template_test.go
and checkLanguage() function changed to templateFolderExists()

Signed-off-by: Eric Stoekl <ems5311@gmail.com>
@ericstoekl

This comment has been minimized.

Show comment
Hide comment
@ericstoekl

ericstoekl Nov 9, 2017

Contributor

@alexellis , just made those requested updates. I've refactored the part in the loop with continue usage to use a separate function which returns an enum that is then used in a switch statement to tell whether to continue with the loop or not. I've also changed the naming of several identifiers as requested, and included the function data into the go-armhf, node-armhf, and python-armhf directories. Let me know what you think. Thanks!

Contributor

ericstoekl commented Nov 9, 2017

@alexellis , just made those requested updates. I've refactored the part in the loop with continue usage to use a separate function which returns an enum that is then used in a switch statement to tell whether to continue with the loop or not. I've also changed the naming of several identifiers as requested, and included the function data into the go-armhf, node-armhf, and python-armhf directories. Let me know what you think. Thanks!

@alexellis alexellis moved this from In review to Read to merge in #TeamServerless Nov 11, 2017

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Nov 11, 2017

Member

Needs a rebase.

Member

alexellis commented Nov 11, 2017

Needs a rebase.

alexellis added a commit that referenced this pull request Nov 11, 2017

template pull - address issues from thread #201
1) Moves code that merges zip file contents into template/ dir into
expandTemplatesFromZip() function. Removes hard-coded constant.
2) Sort list of languages found with 'faas-cli new --list' before
printing
3) Fixes for typos, more descriptive comments, and remove incorrect
license attribution

Signed-off-by: Eric Stoekl <ems5311@gmail.com>
@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Nov 11, 2017

Member

Rebasing and carrying out some review / clean-up over in #220.

Member

alexellis commented Nov 11, 2017

Rebasing and carrying out some review / clean-up over in #220.

alexellis added a commit that referenced this pull request Nov 11, 2017

template pull - address issues from thread #201
1) Moves code that merges zip file contents into template/ dir into
expandTemplatesFromZip() function. Removes hard-coded constant.
2) Sort list of languages found with 'faas-cli new --list' before
printing
3) Fixes for typos, more descriptive comments, and remove incorrect
license attribution

Signed-off-by: Eric Stoekl <ems5311@gmail.com>

@alexellis alexellis closed this Nov 11, 2017

@alexellis alexellis moved this from Read to merge to Done in #TeamServerless Nov 11, 2017

@alexellis alexellis removed this from Done in #TeamServerless Mar 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment