Skip to content
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

Add template pull stack #668

Conversation

@martindekov
Copy link
Member

commented Aug 4, 2019

Adding tempalte pull stack command which will pull tempaltes
from configuration field in the function yaml or from separate
yaml file with that same field

Signed-off-by: Martin Dekov mvdekov@gmail.com

Description

Adding template pull stack command to the cli to pull templates configured in yaml file in field looking the following way:

configuration:
  templates:
    powers: https://github.com/openfaas-incubator/powershell-template
    rusta: https://github.com/openfaas-incubator/openfaas-rust-template

The output is in the following format when --name flag is not present:

Pulling template: `powers` from configuration file: `stack.yml`
Fetch templates from repository: https://github.com/openfaas-incubator/powershell-template at master
2019/08/18 17:17:20 Attempting to expand templates from https://github.com/openfaas-incubator/powershell-template
2019/08/18 17:17:21 Cannot overwrite the following 1 template(s): [powershell]
2019/08/18 17:17:21 Fetched 0 template(s) : [] from https://github.com/openfaas-incubator/powershell-template
Pulling template: `rusta` from configuration file: `stack.yml`
Fetch templates from repository: https://github.com/openfaas-incubator/openfaas-rust-template at master
2019/08/18 17:17:21 Attempting to expand templates from https://github.com/openfaas-incubator/openfaas-rust-template
2019/08/18 17:17:22 Cannot overwrite the following 1 template(s): [rust]
2019/08/18 17:17:22 Fetched 0 template(s) : [] from https://github.com/openfaas-incubator/openfaas-rust-template

When --name flag present it is just the chosen template, when missing, error is shown.

Motivation and Context

  • I have raised an issue to propose this change (required)
    Closes #617
    The configuration field is purposely not added to Services struct. It can be part of the stack.yml or different YAML file at the moment. Read discussion in the issue for context.

How Has This Been Tested?

Manual testing, I mostly used already existing functionality which is unit tested.
Tested the pull command for regression manually.

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.
Copy link
Member

left a comment

I have started testing it manually,

  • It worked as expected when I added this configuration to my stack file

    configuration:
      template:
        sources:
          - name: pydatascience
            url: https://github.com/LucasRoesler/pydatascience-template
    $ faas-cli template pull stack
    Pulling template: pydatascience from configuration
    Fetch templates from repository: https://github.com/LucasRoesler/pydatascience-template at master
    2019/08/11 15:36:41 Attempting to expand templates from https://github.com/LucasRoesler/pydatascience-template
    2019/08/11 15:36:42 Fetched 2 template(s) : [pydatascience pydatascience-web] from https://github.com/LucasRoesler/pydatascience-template
  • It also worked as expected when I added a hash to the URL

  • I am not sure about this error message when the configuration section is empty/missing:

    $ faas-cli template pull stack
    Error while reading configuration: unable to locate template field

    I was expecting something like "no template repos currently saved/configured". Not sure if that is the best error message, but i don't think an empty configuration should be the same as this kind of parsing error.

I left other specific comments as line comments on the diff

stack/schema.go Outdated Show resolved Hide resolved
commands/template_pull_stack.go Outdated Show resolved Hide resolved
stack/schema.go Outdated Show resolved Hide resolved
@martindekov

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2019

Thank you for the review @LucasRoesler you made me think about the implementation approach and I though couple of things which can be done better 💯

@LucasRoesler

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

@martindekov how are things going? I am looking forward to merging this, I have some nice plans for how it will integrate well into CI environments like the new Github Actions

@martindekov

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2019

Hey Lucas I had some work in the weekend I will refer your comments today. Do you need saving logic for your plans ?

@LucasRoesler

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

Saving is of course important, but I am willing to hand write stuff in the mean time :)

@martindekov martindekov force-pushed the martindekov:martindekov/617_list_template_urls_in_stack_yaml branch from 3fb2935 to 820b584 Aug 18, 2019
@martindekov

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2019

Hey @LucasRoesler added the changes, removed the unnecessary nesting, now the templates are in the form name:url instead of the whole nesting and listing, thank you again for the review, feel free to test etc. 🚀 Also updated the description

Copy link
Member

left a comment

@martindekov I am not sure I understand the use of the customName flag and the findTemplate method. Can you give an example of how it should be used? Is it a filter for during pull?

commands/template_pull_stack.go Outdated Show resolved Hide resolved
commands/template_pull_stack.go Show resolved Hide resolved
@martindekov martindekov force-pushed the martindekov:martindekov/617_list_template_urls_in_stack_yaml branch from 820b584 to 8b131eb Aug 19, 2019
@martindekov

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

@LucasRoesler Also added test due to your comment about passing the flag value 👍

@LucasRoesler

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

@martindekov i am sorry to pester, but I am still not sure I really understand how I should use the custom name, it seems like a it is an alias for a template repo, not for the template itself, is that right?

@martindekov

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

It is actually my fault @LucasRoesler, sorry about that, forgot to answer the question, yes if you set custom name, only the template with that key will be downloaded.
So:

$ faas-cli template pull stack --name powers
Pulling template: `powers` from configuration file: `stack.yml`
Fetch templates from repository: https://github.com/openfaas-incubator/powershell-template at master
2019/08/18 17:17:20 Attempting to expand templates from https://github.com/openfaas-incubator/powershell-template
2019/08/18 17:17:21 Cannot overwrite the following 1 template(s): [powershell]
2019/08/18 17:17:21 Fetched 0 template(s) : [] from https://github.com/openfaas-incubator/powershell-template

If you skip the name, all the templates will be downloaded.

@LucasRoesler

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

@martindekov I don't think the name filter is working as expected. I have a template repo with two templates, but if I specify the name filter, I would expect it to only copy one of the templates, right?

➜  classify-name git:(master) ✗ rm -r template
➜  classify-name git:(master) ✗ faas-cli template pull stack --name pydatascience
Pulling template: `pydatascience` from configuration file: `stack.yml`
Fetch templates from repository: https://github.com/LucasRoesler/pydatascience-template at master
2019/09/03 09:41:32 Attempting to expand templates from https://github.com/LucasRoesler/pydatascience-template
2019/09/03 09:41:33 Fetched 2 template(s) : [pydatascience pydatascience-web] from https://github.com/LucasRoesler/pydatascience-template
➜  classify-name git:(master) ✗ ll template
total 0
drwxr-xr-x  7 lucasroesler  staff  224 Sep  3 09:41 pydatascience/
drwxr-xr-x  7 lucasroesler  staff  224 Sep  3 09:41 pydatascience-web/

Other than this, everything works as I would expect it

@martindekov

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

Yeah @LucasRoesler, actually two templates will be pulled, which exist in the repo. In your case this repo has two templates in it in the template folder pydatascience-web and pydatascience.

Adding tempalte pull stack command which will pull tempaltes
from configuration field in the function yaml or from separate
yaml file with that same field

Signed-off-by: Martin Dekov <mvdekov@gmail.com>
@martindekov martindekov force-pushed the martindekov:martindekov/617_list_template_urls_in_stack_yaml branch from 8b131eb to c0b6790 Sep 3, 2019
@martindekov

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

As discussed in the channel, changed the --name flag to --repo flag to make sure there is no ambiguity, along with changing descriptions from template to tempalte repo.

Tested:

$ faas-cli template pull stack --repo powers
Pulling template repo: `powers` from configuration file: `stack.yml`
Fetch templates from repository: https://github.com/openfaas-incubator/powershell-template at master
2019/09/03 19:32:01 Attempting to expand templates from https://github.com/openfaas-incubator/powershell-template
2019/09/03 19:32:04 Cannot overwrite the following 1 template(s): [powershell]
2019/09/03 19:32:04 Fetched 0 template(s) : [] from https://github.com/openfaas-incubator/powershell-template
$ faas-cli template pull stack --repo asdf
Unable to find template repo with name: `asdf`
$ faas-cli template pull stack --help
Downloads templates specified in the function yaml file, in the current directory

Usage:
  faas-cli template pull stack [flags]

Examples:

  faas-cli template pull stack
  faas-cli template pull stack -f myfunction.yml
  faas-cli template pull stack -r custom_repo_name


Flags:
      --debug         Enable debug output
  -h, --help          help for stack
      --overwrite     Overwrite existing templates?
  -r, --repo string   The custom name of the template repo

Global Flags:
      --filter string   Wildcard to match with function names in YAML file
      --regex string    Regex to match with function names in YAML file
  -f, --yaml string     Path to YAML file describing function(s)
Copy link
Member

left a comment

@alexellis looks good to me

@alexellis

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

I appreciate @martindekov working hard on this and the other people reviewing the code.

I can't get to this yet, but I want to play with it and see if it makes sense before making it permanent.

I am still slightly uncomfortable with this being in stack.yml, I feel like it belongs outside and the save feature adds weight to that. I may be wrong, I've been wrong before.

Alex

@martindekov

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2019

Yeah, of course, no problems, when you can. Ping me when you have solid sight on this feature.

Saving would move out the logic from stack.yml into template.yaml or similar and I believe it is a stand alone feature which will require augmenting the faas-cli new command, to create the file mentioned above and save the templates pulled, along with adding additional command to handle the saving itself.

The current change is MVP and only enables you to have your templates listed in a yaml file, instead of faas-cli template pull <some_time_to_find_the_url>.(I am creating TL:DR notes with those comments btw). We might wait for the yaml.v3 Node to mature a little bit and start using it, possibly sticking to stack.yml

}

type StackConfiguration struct {
TemlpatesConfigs map[string]string `yaml:"templates"`

This comment has been minimized.

Copy link
@alexellis

alexellis Oct 8, 2019

Member

Double plural is not very natural. what can you suggest instead?

TemplateConfigs?

Temlpates is also a typo.

@alexellis

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

What about priority - i.e. check store first, then check for a URL?

I would expect that I could do something like:

configuration:
  templates:
    node10-express: store
    rust: https://github.com/openfaas-incubator/openfaas-rust-template

or?

configuration:
  templates:
    - name: node10-express
# no URL means fetch from URL in the store.
    - name: rust
      url: https://github.com/openfaas-incubator/openfaas-rust-template
@alexellis

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

See my original comment and design suggestion:

#617 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.