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

use template store pull in build for fetching the configured template before default templates #965

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NikhilSharmaWe
Copy link
Contributor

Description

This PR makes the faas-cli fetch the template specified in the config yaml of a function before pulling the default templates from https://github.com/openfaas/templates.git.

Motivation and Context

How Has This Been Tested?

Demo: https://asciinema.org/a/5caWeeVN1fDHWZ8gNqU6GADi5

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.

… first

Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
@NikhilSharmaWe NikhilSharmaWe changed the title use template store pull in build for fetching the configured template first use template store pull in build for fetching the configured template before default templates Jun 1, 2023
@NikhilSharmaWe
Copy link
Contributor Author

@alexellis PTAL

@alexellis
Copy link
Member

Hi @NikhilSharmaWe will do. Could you format your commit message please?

https://cbea.ms/git-commit/

@alexellis
Copy link
Member

It's not what I was asking for, but this behaviour may be better.

We also need to handle the case where someone's set up the configuration section in the YAML file to specify which templates to download.

See the note here: #927 (comment)

That section needs to be considered.

This works because golang-middleware is in the template store to - but a custom template may not be in the store - therefore, you'd have the URL in the config section.. and the CLI would need to iterate / check there and download those templates (if required).

@NikhilSharmaWe
Copy link
Contributor Author

@alexellis In this : #927 (comment), you are using

config:
  templates:
  - name: golang-middleware

instead of

configuration:
  templates:
  - name: golang-middleware

Here I try it out with no source and with a custom source, and it works. PTAL: https://asciinema.org/a/OWfpfF1AQe39tgB1km5zu1tX3

Please give your thoughts.

@alexellis
Copy link
Member

Interesting... I wonder whether that was the issue all along - that it worked, but I was giving the wrong name for the section in the YAML.

But I also like the idea of looking at the store for any templates that are not found in a local directory.

@NikhilSharmaWe NikhilSharmaWe reopened this Jun 2, 2023
@NikhilSharmaWe NikhilSharmaWe marked this pull request as ready for review June 2, 2023 08:37
@NikhilSharmaWe
Copy link
Contributor Author

@alexellis Please inform me if just in case something else needs to be added here.

@alexellis
Copy link
Member

We need to have several people play with this, and ideally draw up a truth table of how the command works in different scenarios.

No templates locally, no config section, named function not in store
No templates locally, no config section, named function in store
No templates locally, config section present, named function in store

This is at least 3x3 configurations to evaluate.

if pullErr := pullTemplates(templateAddress); pullErr != nil {
return fmt.Errorf("could not pull templates for OpenFaaS: %v", pullErr)
for _, function := range services.Functions {
runTemplateStorePull(cmd, []string{function.Language})
Copy link
Contributor Author

@NikhilSharmaWe NikhilSharmaWe Jun 4, 2023

Choose a reason for hiding this comment

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

@alexellis Just informing you that I'm a little busy with my exams this week, so I'll be able to test the scenarios for these changes after this week.

Thank you

@NikhilSharmaWe
Copy link
Contributor Author

@alexellis Any updates regarding this?
Something need to be done here?

@NikhilSharmaWe
Copy link
Contributor Author

NikhilSharmaWe commented Jun 20, 2023

@alexellis PTAL at my previous comment.

@alexellis
Copy link
Member

I left a comment, I'm not sure if you've seen it yet? #965 (comment)

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.

[Feature Request] Fetch templates from configuration first
2 participants