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 stack file template configuration to build #726

Conversation

martindekov
Copy link
Contributor

@martindekov martindekov commented Nov 12, 2019

Adding functionality which will pull templates defined in the
function YAML file. Additionaly added overwrite flag to the
build command in order to enable the command to overwrite
templates. Also removed the filtering funcitonality from template
pull stack command and renamed the method which will be used to
pull the configuration

Signed-off-by: Martin Dekov [email protected]

Configuration in stack.yml file was used only by template pull stack command. Now the configuration is additionally red in the building phase of the function.

Description

Adding --overwrite flag to the faas-cli build command as it reads and pull templates
defined in the function YAML file. Removing the filtering logic of the template pull stack
along with its testing as requested and renaming the method responsible for the pulling.

Motivation and Context

  • I have raised an issue to propose this change (required)

Closes #725

How Has This Been Tested?

Appended to a new function's stack.yml the following:

configuration:
  templates:
    - name: perl-alpine
    - name: rusta
      source: https://github.com/openfaas-incubator/openfaas-rust-template

and tested

  • faas-cli build - pulled templates if not there, if there used them cached
  • faas-cli build --overwrite - pulled templates no matter present or not
  • faas-cli template pull stack - same as above
  • faas-cli template pull stack --overwrite - same as above

Additional case when there are already existing templates(perl-alpine,rust,valum-http) and we decide to add additional template(crystal-http) to the configuration the following happens.

$ faas-cli build 
Pulling template: perl-alpine from configuration file: stack.yml
Fetch templates from repository: https://github.com/tmiklas/openfaas-perl-templates at master
2019/11/16 13:21:32 Attempting to expand templates from https://github.com/tmiklas/openfaas-perl-templates
2019/11/16 13:21:34 Cannot overwrite the following 1 template(s): [perl-alpine]
2019/11/16 13:21:34 Fetched 0 template(s) : [] from https://github.com/tmiklas/openfaas-perl-templates
Pulling template: rusta from configuration file: stack.yml
Fetch templates from repository: https://github.com/openfaas-incubator/openfaas-rust-template at master
2019/11/16 13:21:34 Attempting to expand templates from https://github.com/openfaas-incubator/openfaas-rust-template
2019/11/16 13:21:36 Cannot overwrite the following 1 template(s): [rust]
2019/11/16 13:21:36 Fetched 0 template(s) : [] from https://github.com/openfaas-incubator/openfaas-rust-template
Pulling template: vala-http from configuration file: stack.yml
Fetch templates from repository: https://github.com/affix/openfaas-templates-affix/ at master
2019/11/16 13:21:36 Attempting to expand templates from https://github.com/affix/openfaas-templates-affix/
2019/11/16 13:21:37 Cannot overwrite the following 4 template(s): [lua53 swift vala-valum vala-valum-http]
2019/11/16 13:21:37 Fetched 0 template(s) : [] from https://github.com/affix/openfaas-templates-affix/
Pulling template: crystal-http from configuration file: stack.yml
Fetch templates from repository: https://github.com/koffeinfrei/crystal-http-template at master
2019/11/16 13:21:37 Attempting to expand templates from https://github.com/koffeinfrei/crystal-http-template
2019/11/16 13:21:39 Fetched 1 template(s) : [crystal-http] from https://github.com/koffeinfrei/crystal-http-template

Removed the appended template configuration and re-ran the tests mentioned above and the building process was as it were without the change.

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.

@martindekov martindekov force-pushed the martindekov/725_add_template_stack_to_build branch from 6533820 to 8b3bff9 Compare November 12, 2019 18:00
@rgee0
Copy link
Contributor

rgee0 commented Nov 12, 2019

I'm not convinced about the UX here. I know the issue said pull the templates only if the template folder doesn't exist but lets consider where the template folder does exist but the language template isn't contained within. Should this work without overwrite? As its non-destructive it feels like it ought to.

@martindekov
Copy link
Contributor Author

Hey Richard, I can remove overwrite flag from the build command and in case you want to overwrite something you can be explicit and use template pull stack --overwrite. Would that be better? WDYT?

@rgee0
Copy link
Contributor

rgee0 commented Nov 12, 2019

Possibly (having an --overwrite flag on build does seem a little unintuitive). I think what i was thinking more of was transparently pulling the stack's templates on build in scenarios where the stack's language folder(s) don't already exist. So, as an example, we could have a situation where the standard templates are present and the stack is using two non-standard templates, here faas build should not clobber the existing templates and should successfully build without the user having to provide the --overwrite flag

@martindekov
Copy link
Contributor Author

martindekov commented Nov 13, 2019

I agree with you @rgee0 having overwrite on build is kind of confusing. I will probably remove the flag for this command. Otherwise if the template folder exists and we add another template to the configuration only the new templates will be pulled on build. I believe Lucas has the same question in his mind. And following the logic it works without overwrite 👍 I apologize for misreading the question first time, I had a ton of things going around at that moment and at those moments your brain just sings a song to keep you form concentrating 😄

@martindekov martindekov force-pushed the martindekov/725_add_template_stack_to_build branch from 8b3bff9 to 86b97f1 Compare November 16, 2019 11:19
@martindekov
Copy link
Contributor Author

Removed --overwrite and updated the examples to show that if we add additional template it will just be pulled additionally, the rest of the templates will not be overwritten.

Copy link
Member

@LucasRoesler LucasRoesler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This worked for me and i think it looks good.

Also, i think once this is merged, i can add the "common" folder support to the StackConfiguration as well

@martindekov
Copy link
Contributor Author

Not familiar with common folder. Is there an Issue? I am curious, also I think we can stick with the configuration in stack.yml saw there is update in yaml.v3 the most interesting one being the better handling of comments I need to test it out though. Possibly lookin forward save stack.

@LucasRoesler
Copy link
Member

@martindekov feel free to ignore my comment about common folder, it is a separate feature, but the configuration will go inside StackConfiguration

@rgee0
Copy link
Contributor

rgee0 commented Nov 17, 2019

#322 is the common folder discussion

@alexellis
Copy link
Member

@martindekov a rebase is required for this PR.

Could you also add a flag that can be used by OpenFaaS Cloud and git-tar to turn off the custom template behaviour? i.e. --disable-stack-pull?

@martindekov
Copy link
Contributor Author

Yeah Alex no problems 👍

Adding functionality which will pull templates defined in the
function YAML file.  Also removed the filtering funcitonality
from template pull stack command and renamed the method which
will be used to pull the configuration

Signed-off-by: Martin Dekov <[email protected]>
@martindekov martindekov force-pushed the martindekov/725_add_template_stack_to_build branch from 86b97f1 to ac60b4f Compare December 10, 2019 18:59
@martindekov
Copy link
Contributor Author

Done 👍

Tested stack.yml with content:

...
    image: martindekov/saws:latest

configuration:
  templates:
    - name: rusta
      source: https://github.com/openfaas-incubator/openfaas-rust-template
    - name: vala-http
    - name: crystal-http
$ faas-cli build
Pulling template: rusta from configuration file: stack.yml
Fetch templates from repository: https://github.com/openfaas-incubator/openfaas-rust-template at master
2019/12/10 20:58:22 Attempting to expand templates from https://github.com/openfaas-incubator/openfaas-rust-template
2019/12/10 20:58:24 Cannot overwrite the following 1 template(s): [rust]
2019/12/10 20:58:24 Fetched 0 template(s) : [] from https://github.com/openfaas-incubator/openfaas-rust-template
Pulling template: vala-http from configuration file: stack.yml
Fetch templates from repository: https://github.com/affix/openfaas-templates-affix/ at master
2019/12/10 20:58:25 Attempting to expand templates from https://github.com/affix/openfaas-templates-affix/
2019/12/10 20:58:26 Cannot overwrite the following 4 template(s): [lua53 swift vala-valum vala-valum-http]
2019/12/10 20:58:26 Fetched 0 template(s) : [] from https://github.com/affix/openfaas-templates-affix/
Pulling template: crystal-http from configuration file: stack.yml
Fetch templates from repository: https://github.com/koffeinfrei/crystal-http-template at master
2019/12/10 20:58:26 Attempting to expand templates from https://github.com/koffeinfrei/crystal-http-template
2019/12/10 20:58:28 Cannot overwrite the following 1 template(s): [crystal-http]
2019/12/10 20:58:28 Fetched 0 template(s) : [] from https://github.com/koffeinfrei/crystal-http-template
[0] > Building newfuncyaw.
[0] < Building newfuncyaw done in 0.00s.
[0] > Building saws.
Clearing temporary build folder: ./build/saws/
Preparing ./saws/ ./build/saws//function
Building: martindekov/saws:latest with python template. Please wait..
...

And with flag --disable-stack-pull present:

$ faas-cli build --disable-stack-pull
[0] > Building newfuncyaw.
[0] < Building newfuncyaw done in 0.00s.
[0] > Building saws.
Clearing temporary build folder: ./build/saws/
Preparing ./saws/ ./build/saws//function
Building: martindekov/saws:latest with python template. Please wait..
Sending build context to Docker daemon  8.192kB
...

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@alexellis alexellis merged commit 39a7a0b into openfaas:master Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additional template configuration not used at build time
4 participants