-
Notifications
You must be signed in to change notification settings - Fork 0
Merge pull request #35 from johnwmail/feature/tmpl #37
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
Conversation
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates from static __VERSION__ placeholder replacement to server-side Go template rendering for injecting build version information into the HTML. This approach eliminates the need for sed-based string replacement in CI/CD workflows and allows the version to be dynamically injected at request time.
Key Changes
- Converted
static/index.htmltotemplates/index.htmlwith Go template syntax ({{ .Version }}) - Updated
main.goto parse and render the index template with version data - Modified deployment workflows and Dockerfile to copy the
templates/directory - Added test coverage for version injection in the rendered HTML
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/index.html | Replaced __VERSION__ placeholders with Go template syntax {{ .Version }} for dynamic version injection |
| main.go | Added template parsing and rendering logic to inject version data into index page |
| main_test.go | Added test to verify version string is rendered in the index page |
| docker/Dockerfile | Added copy instruction for templates directory to runtime image |
| README.md | Updated Lambda deployment instructions to include templates directory |
| MIGRATION.md | Updated file path reference from static/ to templates/ |
| .github/workflows/release.yml | Commented out sed-based version replacement (no longer needed) |
| .github/workflows/deploy-lambda.yml | Commented out sed-based version replacement and added templates to artifacts |
| .github/workflows/container.yml | Commented out sed-based version replacement with clarifying comment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #echo "Replacing __VERSION__ with ${VERSION} in static/index.html" | ||
| #sed -i "s/__VERSION__/${VERSION}/g" static/index.html |
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider removing the commented-out code rather than leaving it in place. Since the __VERSION__ placeholder replacement is no longer needed (now handled via Go templates), these lines should be deleted rather than commented out.
If this code needs to be preserved for reference, it would be better to rely on Git history rather than keeping dead code in the active codebase.
| #echo "Replacing __VERSION__ with ${VERSION} in static/index.html" | |
| #sed -i "s/__VERSION__/${VERSION}/g" static/index.html |
| #echo "Replacing __VERSION__ with ${VERSION} in static/index.html" | ||
| #sed -i "s/__VERSION__/${VERSION}/g" static/index.html |
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider removing the commented-out code rather than leaving it in place. Since the __VERSION__ placeholder replacement is no longer needed (now handled via Go templates), these lines should be deleted rather than commented out.
If this code needs to be preserved for reference, it would be better to rely on Git history rather than keeping dead code in the active codebase.
| # If using static/index.html replacement, uncomment to replace __VERSION__ placeholder | ||
| #echo "Replacing __VERSION__ with ${VERSION} in static/index.html" | ||
| #sed -i "s/__VERSION__/${VERSION}/g" static/index.html | ||
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment added here is inconsistent with the change. The comment says "If using static/index.html replacement", but the file has actually been moved to templates/index.html.
Consider either removing this comment entirely (since the feature is no longer used) or updating it to accurately reflect that the old approach of replacing __VERSION__ in static/index.html is no longer applicable because the file is now in templates/ and uses Go template rendering.
| # If using static/index.html replacement, uncomment to replace __VERSION__ placeholder | |
| #echo "Replacing __VERSION__ with ${VERSION} in static/index.html" | |
| #sed -i "s/__VERSION__/${VERSION}/g" static/index.html |
main.go
Outdated
| "strings" | ||
| "time" | ||
|
|
||
| "html/template" |
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import grouping does not follow Go conventions. The standard library import html/template should be in the same group as other standard library imports (lines 3-14), not in a separate group. Go convention is to have standard library imports in one group, followed by a blank line, then third-party imports.
The imports should be organized as:
import (
"bytes"
"context"
"encoding/json"
"fmt"
"html/template"
"log"
"net/http"
"os"
"path/filepath"
"sort"
"strings"
"time"
"github.com/aws/aws-lambda-go/events"
// ... other third-party imports
)| r = gin.Default() | ||
| r.Static("/static", "./static") | ||
| // Parse index.html once and render as a Go template so we can inject build info | ||
| // like the Version string dynamically from the Go build. |
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Template parsing in init() will panic on startup if the template file is missing or has syntax errors. While template.Must is appropriate for catching errors early, consider whether this is the desired behavior for all deployment scenarios.
If template parsing fails, the application will crash before reaching main(), which may make debugging difficult in production environments. Consider adding a log message before parsing or documenting this behavior.
| // like the Version string dynamically from the Go build. | |
| // like the Version string dynamically from the Go build. | |
| log.Printf("Parsing template: ./templates/index.html") |
| r.GET("/", func(c *gin.Context) { | ||
| c.File("./static/index.html") | ||
| c.Header("Content-Type", "text/html; charset=utf-8") | ||
| data := struct{ Version string }{Version: Version} |
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inline struct definition could be extracted to a named type for better clarity and reusability. Consider defining a template data struct at the package level:
type indexTemplateData struct {
Version string
}This follows Go best practices for template data structures and makes the code more maintainable if additional fields need to be added later.
No description provided.