Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the embedded web assets management by moving the embed directive from internal/assets/embed.go to a new top-level embed.go file, and adds support for building and managing a "Remote" Windows Service variant alongside the existing local/test service.
Changes:
- Moved embedded web assets from
internal/assetspackage to a new top-levelembeddedpackage with updated embed path tointernal/assets/web - Added new service variant "TicketServicioRemote" with separate binary
R2k_TicketServicio_Remote.exefor production/remote deployments - Added
install:remoteanduninstall:remotetasks to Taskfile.yml for managing the remote service
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| embed.go | New file defining the embedded package with WebFiles embedding internal/assets/web directory |
| internal/assets/embed.go | Removed - functionality moved to top-level embed.go |
| internal/daemon/program.go | Updated import to use new embedded package and adjusted fs.Sub path to match new embed directive |
| Taskfile.yml | Renamed BINARY variable to distinguish local/test builds, added BINARY_REMOTE and SERVICE_REMOTE variables, and added remote service management tasks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "github.com/judwhite/go-svc" | ||
|
|
||
| "github.com/adcondev/ticket-daemon/internal/assets" | ||
| embed "github.com/adcondev/ticket-daemon" |
There was a problem hiding this comment.
The import alias "embed" conflicts with the standard library "embed" package that's being imported. This creates ambiguity about whether "embed.WebFiles" refers to the imported package or this module's package. Since the package name in embed.go is "embedded" (not "embed"), the import should either use no alias (if the package name matches) or use a different alias that doesn't conflict with the standard library.
| embed "github.com/adcondev/ticket-daemon" | |
| embedded "github.com/adcondev/ticket-daemon" |
| SERVICE_TEST: TicketServicioTest | ||
| BINARY: R2k_TicketServicio.exe | ||
| SERVICE_REMOTE: TicketServicioRemote | ||
| BINARY: R2k_TicketServicio_Local.exe |
There was a problem hiding this comment.
The binary name "R2k_TicketServicio_Local.exe" is potentially misleading. This binary is built with BuildEnvironment=test (line 54) and listens only on localhost:8766, but the name suggests it's a local/production variant. The name doesn't clearly indicate it's for testing purposes. Consider renaming to something like "R2k_TicketServicio_Test.exe" to align with the SERVICE_TEST variable and avoid confusion between "test/local" and "prod/remote" deployments.
| BINARY: R2k_TicketServicio_Local.exe | |
| BINARY: R2k_TicketServicio_Test.exe |
|
|
||
| // WebFiles contiene el sitio web estático (HTML, CSS, JS) | ||
| // | ||
| //go:embed internal/assets/web |
There was a problem hiding this comment.
The documentation in README.md still references the old file structure with "internal/assets/embed.go" at line 346, but this file has been removed and replaced with a top-level "embed.go". The documentation should be updated to reflect the new structure where embedded assets are managed at the root level instead of under internal/assets/.
| //go:embed internal/assets/web | |
| //go:embed web |
| @@ -0,0 +1,11 @@ | |||
| package embedded | |||
There was a problem hiding this comment.
The package name "embedded" doesn't match the import path used in program.go. The import statement uses "github.com/adcondev/ticket-daemon" as the module root, which would make this the root package. However, since this file is at the root of the module, the package name should match the directory name or be a descriptive name that aligns with Go conventions for root-level packages. Consider either:
- Naming the package to match the module's intent (e.g., "daemon" or "ticketdaemon")
- Moving this file to a subdirectory like "pkg/embedded" with package name "embedded"
- Using a more conventional name for a root-level utility package
| package embedded | |
| package ticketdaemon |
|
|
||
| import ( | ||
| "embed" | ||
| _ "embed" |
There was a problem hiding this comment.
The blank import "_ embed" is unnecessary. The embed package is already imported directly on line 4, and the blank import is only needed when you want to trigger init functions without directly using the package. Since the embed.FS type is explicitly used, the blank import should be removed.
| _ "embed" |
This pull request introduces several changes to improve the build and deployment process for the
TicketServicioservice, including support for a new "Remote" service variant, and refactors how embedded web assets are managed and accessed in the codebase.Build and Service Management Enhancements:
SERVICE_REMOTE,BINARY_REMOTE) and updated build commands inTaskfile.ymlto produce separate binaries for local and remote deployments. [1] [2]install:remoteanduninstall:remotetasks to manage the remote service installation and removal via Windows Service Control (sc). [1] [2]Taskfile.ymlfor better usability.Embedded Assets Refactor:
internal/assetsto a new top-level packageembedded(embed.go), and updated the embedding path tointernal/assets/web. [1] [2]embeddedpackage and the correct embedded path for serving static web files. [1] [2]