Version pinning for templates #1012
Conversation
alexellis
force-pushed
the
pin-template
branch
2 times, most recently
from
4e0cc29 to
e88116b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements version pinning for templates, allowing functions to specify different versions of templates using three methods: using latest/head, a specific branch or git tag, or a specific SHA hash. The syntax uses the @ symbol (e.g., [email protected] or golang-middleware@sha-2e6e262).
Key Changes:
- Added support for version-pinned template references using
@syntax in function language specifications - Implemented SHA-based checkout for templates with
sha-prefix - Changed default
--overwriteflag behavior fromfalsetotruefor template pull commands - Refactored template pulling logic to handle missing templates and version references
Reviewed Changes
Copilot reviewed 37 out of 39 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| versioncontrol/parse.go | Fixed typo: pinCharater → pinCharacter; updated parsing logic |
| versioncontrol/parse_test.go | Updated tests to use corrected pinCharacter constant |
| versioncontrol/git.go | Added GitCloneBranch, GitCloneFullDepth, and GetGitSHAFor functions |
| versioncontrol/core.go | Added debug logging for git commands |
| commands/fetch_templates.go | Implemented SHA checkout logic, template metadata writing, and version suffix handling |
| commands/template_pull.go | Changed default overwrite to true; added silence flags |
| commands/template_pull_stack.go | Refactored to use getMissingTemplates and handle versioned templates |
| commands/template_store_pull.go | Added version reference extraction and handling |
| commands/new_function.go | Added support for versioned template references in new function creation |
| commands/build.go | Integrated missing template detection and pulling with version support |
| commands/publish.go | Added template pulling logic with version support |
| builder/build.go | Extracted template lookup into getTemplate function |
| builder/copy.go | Improved error handling for directory creation |
| testing/* | Added test configuration files and function handlers |
| config/config_file.go | Improved error handling for directory creation |
| commands/local_run.go | Improved error handling for secrets directory creation |
| commands/plugin_get.go | Fixed error check: os.ErrExist != err → !os.IsExist(err) |
| commands/watch.go | Removed unused blank identifier in range loop |
| commands/secret_apply.go | New command for applying secrets from .secrets folder |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if canWriteLanguage(existingLanguages, language, overwriteTemplate) { | ||
| // Do cp here | ||
| languageSrc := filepath.Join(extractedPath, TemplateDirectory, language) | ||
| languageDest := filepath.Join(localTemplatesDir, language) | ||
| langName := language | ||
| if refName != "" { | ||
| languageDest += "@" + refName | ||
| langName = language + "@" + refName | ||
| } |
There was a problem hiding this comment.
The logic for handling versioned templates appears incomplete. When a template name includes a version suffix (e.g., [email protected]), the destination path is set to language+"@"+refName (line 206), but getMissingTemplates in template_pull_stack.go checks for function.Language which would be the full versioned name. This means the template directory structure uses @ in directory names, but there's no clear handling of how to look up these versioned templates when building. The getTemplate function in builder/build.go looks for exact directory matches by lang name, which may not work correctly with version suffixes.
There was a problem hiding this comment.
getTemplate uses the expanded name so matches the directory exactly.
commands/template_pull_stack.go
Outdated
| } | ||
| return pullStackTemplates(templatesConfig, cmd) | ||
|
|
||
| return pullStackTemplates([]string{}, templatesConfig, cmd) |
There was a problem hiding this comment.
The function signature has changed from pullStackTemplates(templateInfo []stack.TemplateSource, cmd *cobra.Command) to pullStackTemplates(missingTemplates []string, templateSources []stack.TemplateSource, cmd *cobra.Command). However, the first parameter name missingTemplates suggests it should be a filtered list of missing templates, but at line 44 an empty slice is passed. This inconsistency could indicate a logic error or misunderstanding of the function's purpose.
| return pullStackTemplates([]string{}, templatesConfig, cmd) | |
| // Read stack config to get functions map | |
| stackConfig, err := readStackConfig() | |
| if err != nil { | |
| return err | |
| } | |
| // Determine missing templates | |
| missingTemplates, err := getMissingTemplates(stackConfig.StackConfig.Functions, "./template") | |
| if err != nil { | |
| return err | |
| } | |
| return pullStackTemplates(missingTemplates, templatesConfig, cmd) |
| func getMissingTemplates(functions map[string]stack.Function, templatesDir string) ([]string, error) { | ||
| var missing []string | ||
|
|
||
| for _, function := range functions { | ||
| templatePath := fmt.Sprintf("%s/%s", templatesDir, function.Language) | ||
| if _, err := os.Stat(templatePath); err != nil && os.IsNotExist(err) { | ||
| newTemplates = append(newTemplates, info) | ||
| missing = append(missing, function.Language) | ||
| } | ||
| } | ||
|
|
||
| return newTemplates, nil | ||
| return missing, nil |
There was a problem hiding this comment.
The function name getMissingTemplates returns a list of strings representing missing template names, but the function iterates over functions and checks their Language property. This means the function assumes that each function's language directly maps to a template name. However, with the new version pinning feature (e.g., [email protected]), the language field may include version suffixes. The function doesn't handle extracting the base template name from versioned language specifications, which could cause template lookups to fail.
There was a problem hiding this comment.
template.yml cannot contain an @ suffix.
Templates can now be pinned in three ways: - no pinning - get HEAD from default branch - Git tag / branch name - pass @ then the release i.e. 0.9.0 - Git SHA - requires full depth clone of whole respository followed by a git checkout Tested using the following: * Via build and publish with a templates configuration in stack.yaml * Via build and publish with no templates configuration in stack.yaml - the templates were resolved via the store * Via new - with and without a template being present - the template is pulled from the store if not found locally Signed-off-by: Alex Ellis (OpenFaaS Ltd) <[email protected]>
alexellis
force-pushed
the
pin-template
branch
from
e88116b to
dd0b9b1
Compare
|
Need to document:
|
AI Pull Request OverviewSummary
Summary per fileSummary per file
Overall AssessmentOverall AssessmentThis PR successfully implements template version pinning, a valuable feature for ensuring reproducible builds and avoiding breaking changes from template updates. The implementation follows Go and CLI best practices, with good error handling and testing. However, there are some gaps in the stack-based template pulling for versioned templates, and the feature's impact on existing workflows needs careful consideration. Strengths:
Concerns:
Recommendations:
Detailed ReviewDetailed ReviewArchitecture and DesignPositive Aspects:
Issues:
Recommendation: Either:
Code Qualityfetch_templates.go Changes:
build.go Refactoring:
Error Handling:
TestingCoverage:
Gaps:
Performance Considerations
Security
Backward Compatibility
Documentation
Specific Code Commentscommands/fetch_templates.go:52-67 if len(refName) > 0 && strings.HasPrefix(refName, ShaPrefix) {
targetCommit := strings.TrimPrefix(refName, ShaPrefix)
// ... validation and checkout
}
commands/new_function.go:143-147 _, ref, _ := strings.Cut(language, "@")
if info.TemplateName == language || (info.TemplateName+"@"+ref == language) {
versioncontrol/parse.go:56 remoteURL, refName, _ = strings.Cut(repoURL, pinCharacter)
Recommendations for Future Work
Overall, this is a solid implementation of an important feature. The main blocker is the incomplete stack pull support for versioned templates, which should be addressed before merging. AI agent details. |
Signed-off-by: Han Verstraete (OpenFaaS Ltd) <[email protected]>
AI Pull Request OverviewSummary
Summary per fileSummary per file
Overall AssessmentOverall AssessmentThe PR successfully adds template versioning functionality, addressing the risk of unexpected breaking changes in templates. The implementation provides three pinning methods (tag/branch, SHA) and integrates well with existing store and custom source workflows. The versioncontrol package adds robust git ref handling. However, the implementation has critical gaps in custom source pinning support, leading to inconsistent behavior and potential runtime failures when @ refs are used with configured template sources. Detailed ReviewDetailed ReviewCustom Source Pinning Logic FlawsIn For example, with: configuration:
templates:
- name: golang-middleware
source: https://github.com/openfaas/golang-http-template
functions:
go-090:
lang: [email protected]The function will attempt to pull from the store instead of applying the @0.9.0 ref to the custom source. Directory Path Resolution Issues
Incomplete @ to # Conversion
Test Coverage Deficiencies
SHA Pinning Performance ImpactSHA pinning requires full repository cloning ( Recommendations
The core functionality is sound and the feature addresses a real user need. With the identified fixes, this will provide robust template versioning across all supported scenarios. AI agent details. |
Description
Version pinning for templates
Motivation and Context
It's risky for us to make breaking changes are made to templates.
Versioning is available for all the functions within a folder by adding a templates.configuration section along with a custom source and a
#v0.1.0fragment to the git URL.But it isn't possible to have different functions pinned to different versions of templates.
For instance:
1 - Use "head"/latest (as now)
2 - Use a specific branch or git tag aka "release"
3 - Use a specific SHA hash from a commit in HEAD/master
Testing with a templates source given
Testing without a templates source given
Testing via
newwith a store templaterm -rf template ; ./faas-cli new --lang python3-http --prefix ttl.sh/alexellis pyfn1How Has This Been Tested?
The above YAML
Types of changes
This affects:
new,build,publish,template pullChecklist:
Documentation will need an update, but for most users there will be no change unless they opt into versioning.