Pull templates from store when not present #1004
Merged
+227
−174
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implements a user-experience changes: faas-cli new - when the templates folder is not present the language will be looked up in the template store using the default URL or an overriden one. faas-cli publish - when no template folder is available then a pull will be carried out if a configuration section is present with a template given within in Filtering - whilst there is no way to avoid pulling all the templates wihtin a repository, only the requested/needed ones are expanded by commands like faas-cli new. Tested mainly with existing-unit tests, faas-cli new / publish with and without a templates folder. Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
alexellis
commented
Jan 23, 2025
commands/build.go
Outdated
| log.Println("No templates found in current directory.") | ||
| func pullTemplates(templateURL, templateName string) error { | ||
|
|
||
| if _, err := os.Stat("./template"); err != nil && os.IsNotExist(err) { |
There was a problem hiding this comment.
More idiomatic way to check for the folder.
There was a problem hiding this comment.
Agree.
Would os.Stat return any errors other than os.IsNotExist?
There was a problem hiding this comment.
In a rare case there may be a permissions issue
There was a problem hiding this comment.
Addressed in subsequent commit
alexellis
commented
Jan 23, 2025
| if len(templateURL) == 0 { | ||
| return fmt.Errorf("pass valid templateURL") | ||
| } | ||
|
|
||
| dir, err := os.MkdirTemp("", "openfaas-templates-*") | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| return err |
There was a problem hiding this comment.
Better to return an error than use Fatal
alexellis
commented
Jan 23, 2025
| @@ -56,7 +56,7 @@ func fetchTemplates(templateURL string, refName string, overwrite bool) error { | |||
| log.Printf("Cannot overwrite the following %d template(s): %v\n", len(preExistingLanguages), preExistingLanguages) | |||
| } | |||
|
|
|||
| log.Printf("Fetched %d template(s) : %v from %s\n", len(fetchedLanguages), fetchedLanguages, templateURL) | |||
| fmt.Printf("Wrote %d template(s) : %v from %s\n", len(fetchedLanguages), fetchedLanguages, templateURL) | |||
There was a problem hiding this comment.
No need to be using "log" package here in the CLI output
alexellis
commented
Jan 23, 2025
| "testing" | ||
|
|
||
| "github.com/openfaas/faas-cli/builder" | ||
| "github.com/openfaas/faas-cli/stack" | ||
| ) | ||
|
|
||
| func Test_findTemplate(t *testing.T) { |
There was a problem hiding this comment.
The findTemplate function was not used in any code path so was deleted
The tests relied on ruby being a long term template in the templates repository, however we are moving to a model with a repository per template, and this template has gone to EOL. The Dockerfile template makes sense as a replacement and is still supported. Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
There was a problem hiding this comment.
Copilot reviewed 5 out of 10 changed files in this pull request and generated no comments.
Files not reviewed (5)
- commands/build.go: Evaluated as low risk
- commands/fetch_templates_test.go: Evaluated as low risk
- commands/new_function.go: Evaluated as low risk
- commands/template_pull_stack.go: Evaluated as low risk
- README.md: Evaluated as low risk
Comments suppressed due to low confidence (3)
commands/new_function_test.go:30
- Verify the correctness of the updated regex pattern in the
LangNotExistsOutputstring. Ensure that it matches the expected output format.
LangNotExistsOutput = `(template: "([0-9A-Za-z-])*" was not found in the templates folder or in the store)`
commands/new_function_test.go:275
- Ensure that the changes in the test cases are consistent with the new functionality. Verify that using
dockerfileinstead ofrubyis intentional and correct.
const functionLang = "dockerfile"
commands/template_pull.go:47
- The
templateNamevariable is initialized but not used in therunTemplatePullfunction. This should be reviewed to ensure it's necessary.
templateName := "" // templateName may not be known at this point
Requested by @welteki, if stack.yml is present then it is taken to be the default with a --yaml/-f argument being needed. This change also looks for stack.yaml as a valid alternative name. Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
err could be nil, so it is checked before checking for the type of error. This may not be needed, but is a better default to establish since code is often copied by new contributors when adding features. Addresses comment by @rgee0 for handling error from stat. Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
|
I'm going to merge this so it can get into the hands of a few people, we can make additional changes as we go along. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Pull templates from store when not present
Implements a user-experience changes:
faas-cli new - when the templates folder is not present the language will be looked up in the template store using the default URL or an overriden one.
faas-cli publish - when no template folder is available then a pull will be carried out if a configuration section is present with a template given within in
Filtering - whilst there is no way to avoid pulling all the templates wihtin a repository, only the requested/needed ones are expanded by commands like faas-cli new.
Motivation and Context
Improve user experience as discussed on community call Jan 22 2025
How Has This Been Tested?
Tested mainly with existing-unit tests, faas-cli new / publish with and without a templates folder.
Types of changes