Skip to content

Conversation

@welteki
Copy link
Member

@welteki welteki commented Nov 19, 2025

Description

Fix template handling for faas-cli template pull stack

This PR fixes an issue where templates defined in a stack’s configuration.templates section were not being pulled.

Example configuration in stack.yaml

configuration:
  templates:
    - name: golang-middleware@otel
      source: https://github.com/welteki/golang-http-template#otel
    - name: golang-middleware
      source: https://github.com/welteki/golang-http-template

Expected behaviour:

faas-cli template pull stack should pull each template from its specified source and copy only the template matching the configured name into the template/ directory, even when multiple templates come from the same repository. Templates should be stored using the names defined in the config.

Expected output:

./template
├── golang-middleware
└── golang-middleware@otel

This restores the behaviour of the current faas-cli release.

Motivation and Context

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

Fix a regression for the template pull stack command with template pinning.

How Has This Been Tested?

All existing Unit test pass.
Tested the build, publish and template pull stack command with the provided stack.yaml file and verified the correct templates are fetched.

The template folder is removed between running the different commands.

version: 1.0
provider:
  name: openfaas
  gateway: http://127.0.0.1:8080
functions:
  go-latest:
    lang: golang-middleware
    handler: ./go-latest
    image: ttl.sh/alexellis/go-latest:latest

  go-090:
    lang: [email protected]
    handler: ./go-090
    image: ttl.sh/welteki/go-090:latest

  go-sha-2e6e262:
    lang: golang-middleware@sha-2e6e262
    handler: ./go-sha-2e6e262
    image: ttl.sh/welteki/go-sha-2e6e262:latest

  go-otel:
    lang: golang-otel
    handler: ./go-otel
    image: ttl.sh/welteki/go-otel:latest

configuration:
  templates:
    - name: golang-otel
      source: https://github.com/welteki/golang-http-template#otel
    - name: golang-middleware
      source: https://github.com/welteki/golang-http-template

Running faas-cli template pull stack

Output:

Pulling template: golang-otel from https://github.com/welteki/golang-http-template#otel
2025/11/19 16:15:09 Fetching templates from https://github.com/welteki/golang-http-template [otel]
Wrote 1 template(s) : [golang-otel@otel]
Pulling template: golang-middleware from https://github.com/welteki/golang-http-template
2025/11/19 16:15:09 Fetching templates from https://github.com/welteki/golang-http-template
Wrote 1 template(s) : [golang-middleware]

Resulting template folder:

./template
├── golang-middleware
└── golang-otel

Running faas-cli build --shrinkwrap

Output:

Pulling template: golang-middleware from https://github.com/welteki/golang-http-template
2025/11/19 16:17:18 Fetching templates from https://github.com/welteki/golang-http-template
Wrote 1 template(s) : [golang-middleware]
Pulling template: [email protected] from store
2025/11/19 16:17:18 Fetching templates from https://github.com/openfaas/golang-http-template [0.9.0]
Wrote 2 template(s) : [[email protected] [email protected]]
Pulling template: golang-middleware@sha-2e6e262 from store
2025/11/19 16:17:19 Fetching templates from https://github.com/openfaas/golang-http-template [sha-2e6e262]
Wrote 2 template(s) : [golang-http@sha-2e6e262 golang-middleware@sha-2e6e262]
Pulling template: golang-otel from https://github.com/welteki/golang-http-template#otel
2025/11/19 16:17:20 Fetching templates from https://github.com/welteki/golang-http-template [otel]
Wrote 1 template(s) : [golang-otel@otel]
2025/11/19 16:17:20 Pulled templates: [golang-middleware [email protected] golang-middleware@sha-2e6e262 golang-otel]

Resulting template folder:

./template
├── [email protected]
├── golang-http@sha-2e6e262
├── golang-middleware
├── [email protected]
├── golang-middleware@sha-2e6e262
└── golang-otel

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.

@reviewfn
Copy link

reviewfn bot commented Nov 19, 2025

AI Pull Request Overview

Summary

  • Fixes bug where template pull stack command was not pulling any templates due to passing empty missing templates list.
  • Updates moveTemplates to properly parse and handle template names with version pins (e.g., "node@latest").
  • Adds filtering logic to pull only the specified language when a template name is provided.
  • Revises destination path construction to store templates in versioned folders when applicable.
  • Introduces pullConfigTemplates function to pull all templates from stack configuration.
  • Adds logging output for template fetching operations.
  • Replaces the ineffective pullStackTemplates call with the new pullConfigTemplates in runTemplatePullStack.

Summary per file

Summary per file
File path Summary
commands/fetch_templates.go Modified moveTemplates for pinned template handling and added fetch logging.
commands/template_pull_stack.go Introduced pullConfigTemplates and updated runTemplatePullStack to pull all stack templates.

Overall Assessment

Overall Assessment

The changes effectively address the regression in template pulling functionality introduced by prior template pinning modifications. The fix ensures that faas-cli template pull stack behaves as expected by pulling all configured templates rather than none. Code consistency with existing patterns is maintained, and the handling of version-pinned templates improves robustness. However, the absence of new unit tests for the modified logic poses a risk for future regressions. Additionally, the error handling could be enhanced to provide more granular feedback during template fetching failures. Overall, the implementation is functional but would benefit from test coverage and minor refactoring for clarity.

Detailed Review

Detailed Review

commands/fetch_templates.go

  • Line 155: Addition of template := strings.SplitN(templateName, "@", 2)[0] correctly extracts the base template name, enabling proper filtering and path handling for pinned versions. This aligns with the existing parseTemplateRef logic elsewhere in the codebase.
  • Lines 200-202: The new condition if len(template) > 0 && language != template { continue } ensures only the requested language is processed when a specific template is specified. This prevents unnecessary processing but assumes template matches a language directory name—consider adding validation to handle mismatches gracefully, potentially logging a warning if no matching language is found.
  • Lines 205-213: The updated languageDest logic prioritizes templateName when provided, which correctly supports versioned storage (e.g., "node@latest" folders). However, when templateName is empty, the fallback to language + "@" + refName is appropriate but could be simplified by consolidating path construction logic to avoid duplication.
  • Line 217: Moving langName construction after the languageDest logic is correct, but the conditional if refName != "" { langName = language + "@" + refName } should ensure langName is set to language initially for consistency.
  • Line 284: Adding fmt.Printf("Fetching template %s\n", templateName) provides useful user feedback, consistent with other CLI output in the codebase. Ensure this doesn't introduce performance issues in scripts, though it's minimal.

commands/template_pull_stack.go

  • Line 44: Changing pullStackTemplates([]string{}, templatesConfig, cmd) to pullConfigTemplates(templatesConfig) fixes the core bug where an empty missingTemplates slice resulted in no templates being pulled. This restores pre-pinning behavior as intended.
  • Lines 71-81: The new pullConfigTemplates function iterates over all templateSources and pulls each one, which is straightforward and correct for the stack pull operation. However, it lacks error aggregation—currently, it returns on the first error, potentially leaving some templates unfetched. Consider collecting errors and returning a combined error or continuing with warnings for partial failures.
  • The function reuses pullTemplate with config.Name and config.Source, assuming config.Name is a valid template identifier. This matches the existing API but could benefit from input validation to prevent invalid template names from causing downstream issues.
  • No changes to pullStackTemplates, which remains unused in this context—consider removing it if it's obsolete, or documenting its purpose for missing templates.

General Concerns

  • Testing: The PR indicates all existing tests pass, but no new tests were added for the modified moveTemplates logic or the new pullConfigTemplates function. This is a high-priority gap, as template handling is critical functionality. Add unit tests covering pinned templates, filtering, and error scenarios.
  • Security/Performance: No obvious security issues, but ensure templateName parsing doesn't allow path traversal (e.g., via "../../../"). The changes are lightweight and shouldn't impact performance significantly.
  • Migration/Regression: For existing users, this restores expected behavior, but verify that versioned template storage doesn't conflict with existing unpinned templates. The path changes could lead to duplicate templates if users have mixed pinned/unpinned setups.
  • Code Quality: The logic is functional but could be refactored for readability—e.g., extract path construction into a helper function. Consistent use of filepath.Join throughout would improve maintainability.
  • Edge Cases: Test with templates having no "@" (unpinned), fully pinned, and invalid names. Also, ensure behavior when a template repo contains unexpected language directories.

Prioritize adding comprehensive tests and minor refactoring to prevent future issues.

AI agent details.

Agent processing time: 51.681s
Environment preparation time: 6.661s
Total time from webhook: 1m3.07s

Signed-off-by: Han Verstraete (OpenFaaS Ltd) <[email protected]>
@reviewfn
Copy link

reviewfn bot commented Nov 19, 2025

AI Pull Request Overview

Summary

  • Fixes regression in faas-cli template pull stack command where templates defined in stack configuration were not being pulled
  • Changes the command to pull all configured templates instead of only missing ones
  • Updates template copying logic to handle named templates correctly, ensuring proper naming and filtering
  • Restores expected behavior for template pinning and multiple templates from same repository

Summary per file

Summary per file
File path Summary
commands/fetch_templates.go Adds logic to filter and name templates when pulling specific ones from repositories
commands/template_pull_stack.go Modifies stack template pulling to use new function that pulls all configured templates

Overall Assessment

Overall Assessment

The changes correctly address the reported regression in the template pull stack command. The fix ensures that templates specified in the stack's configuration.templates section are properly pulled and named according to the configuration, supporting template pinning and multiple templates from the same repository.

The implementation is straightforward and maintains backward compatibility. However, there are opportunities for improvement in error handling and test coverage.

Detailed Review

Detailed Review

Code Changes Analysis

commands/template_pull_stack.go:

  • The runTemplatePullStack function now calls pullConfigTemplates instead of pullStackTemplates with an empty missing templates list
  • This fixes the core issue where template pull stack was not pulling any templates because no missing templates were specified
  • The new pullConfigTemplates function iterates through all template sources in the configuration and pulls each one
  • This restores the intended behavior of pulling all configured templates

commands/fetch_templates.go:

  • Added logic in moveTemplates to extract the base template name by splitting on "@"
  • Added filtering to only copy templates that match the requested name when a specific template is provided
  • Modified destination path logic to use the full templateName (including version/pin) for directory naming
  • This ensures templates are stored with correct names like golang-middleware@otel

Strengths

  • The fix directly addresses the root cause of the regression
  • Maintains existing behavior for other template pulling scenarios
  • Code is clean and follows existing patterns
  • Manual testing confirms the fix works as expected

Areas for Improvement

Error Handling:

  • When pulling a specific named template, if no matching template exists in the repository, the function silently succeeds but copies nothing
  • Consider adding validation to ensure at least one template was successfully pulled when a specific name is requested
  • This would provide better feedback to users with misconfigured template sources

Test Coverage:

  • The existing tests cover pullStackTemplates but not the new pullConfigTemplates function
  • Add unit tests for pullConfigTemplates to ensure it correctly handles various template configurations
  • Test edge cases like repositories with no matching templates or invalid configurations

Code Consistency:

  • The pullConfigTemplates function could benefit from more detailed error messages, similar to pullStackTemplates
  • Consider extracting common template pulling logic to reduce duplication between pullConfigTemplates and pullStackTemplates

Security and Performance

  • No security concerns introduced
  • Performance impact is minimal as the changes only affect the explicit template pull stack command
  • Git operations remain the same, so no additional network or disk I/O concerns

Migration and Compatibility

  • Changes are backward compatible
  • Existing stack configurations will work as expected
  • No breaking changes to the CLI interface or behavior

Recommendations

  1. Add error checking in pullConfigTemplates to validate that templates were successfully pulled
  2. Implement unit tests for the new pullConfigTemplates function
  3. Consider adding integration tests that verify the end-to-end template pulling workflow
  4. Update documentation if needed to clarify the behavior of template pull stack vs. automatic template pulling during build

The fix is solid and addresses the immediate issue. The suggested improvements would enhance robustness and maintainability.

AI agent details.

Agent processing time: 48.108s
Environment preparation time: 6.65s
Total time from webhook: 59.487s

@alexellis alexellis merged commit 28524d8 into openfaas:pin-template Nov 19, 2025
2 checks passed
@welteki welteki deleted the fix-pull-stack branch November 19, 2025 16:40
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.

2 participants