Repository navigation
Choose the recorded build registry explicitly - #278
nathanyoung wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new --registry flag to both the build and deploy commands, allowing users to specify a registry URL or ID as the build source. It adds a helper function selectBuildRegistry to resolve the selected registry and updates the build service to use this source registry when registering the container URL. Unit tests are also added to verify the flag and selection logic. Feedback is provided regarding selectBuildRegistry in internal/build/service.go, where returning a pointer to an element of a local slice of structs (&matches[0]) is highlighted as an anti-pattern. It is recommended to store and return pointers to the original elements instead to avoid unnecessary heap allocations and copying.
| func selectBuildRegistry(registries []models.ContainerRegistry, selector string) (*models.ContainerRegistry, error) { | ||
| if len(registries) == 0 { | ||
| return nil, fmt.Errorf("no registry connections found") | ||
| } | ||
| var matches []models.ContainerRegistry | ||
| var choices []string | ||
| for _, registry := range registries { | ||
| choices = append(choices, registry.Url) | ||
| if selector == "" || registry.Id == selector || registry.Url == selector { | ||
| matches = append(matches, registry) | ||
| } | ||
| } | ||
| if len(matches) == 1 { | ||
| return &matches[0], nil | ||
| } | ||
| if len(matches) == 0 { | ||
| return nil, fmt.Errorf("registry %q is not a ready registry for this project; choose --registry from: %s", selector, strings.Join(choices, ", ")) | ||
| } | ||
| return nil, fmt.Errorf("multiple ready registries found; choose the build source with --registry <url-or-id>: %s (the image is uploaded to all ready registries)", strings.Join(choices, ", ")) | ||
| } |
There was a problem hiding this comment.
Returning a pointer to an element of the local matches slice (&matches[0]) is a Go anti-pattern here.
Because matches is a slice of models.ContainerRegistry (struct values), each element in matches is a copy of the original registry. Returning &matches[0] returns a pointer to this temporary copy, forcing the entire matches slice to escape to the heap and causing unnecessary allocations. Furthermore, the returned pointer does not point to the original registry in the registries slice, which could lead to subtle bugs if pointer identity is ever checked.
We can optimize this by iterating over the indices of the original registries slice and storing pointers (*models.ContainerRegistry) in matches. This avoids copying the structs and ensures we return a pointer to the actual registry in the input slice.
func selectBuildRegistry(registries []models.ContainerRegistry, selector string) (*models.ContainerRegistry, error) {
if len(registries) == 0 {
return nil, fmt.Errorf("no registry connections found")
}
var matches []*models.ContainerRegistry
var choices []string
for i := range registries {
registry := ®istries[i]
choices = append(choices, registry.Url)
if selector == "" || registry.Id == selector || registry.Url == selector {
matches = append(matches, registry)
}
}
if len(matches) == 1 {
return matches[0], nil
}
if len(matches) == 0 {
return nil, fmt.Errorf("registry %q is not a ready registry for this project; choose --registry from: %s", selector, strings.Join(choices, ", "))
}
return nil, fmt.Errorf("multiple ready registries found; choose the build source with --registry <url-or-id>: %s (the image is uploaded to all ready registries)", strings.Join(choices, ", "))
}There was a problem hiding this comment.
Addressed in 091c186 using the matched input index and match count. This removes the temporary registry slice and returns the selected input registry without changing upload destinations. Validation: go test ./internal/build ./cmd/build ./cmd/deploy passes. Returning the previous copied value was memory-safe in Go, but the extra slice/copy was unnecessary.
There was a problem hiding this comment.
Code Review
This pull request adds a --registry flag to both the build and deploy commands, allowing users to specify a registry URL or ID as the build source. It also introduces a selectBuildRegistry helper function to resolve the source registry and updates the build service accordingly. The feedback suggests optimizing selectBuildRegistry to avoid temporary slice allocations and struct copying by tracking the matched index and count instead.
| func selectBuildRegistry(registries []models.ContainerRegistry, selector string) (*models.ContainerRegistry, error) { | ||
| if len(registries) == 0 { | ||
| return nil, fmt.Errorf("no registry connections found") | ||
| } | ||
| var matches []models.ContainerRegistry | ||
| var choices []string | ||
| for _, registry := range registries { | ||
| choices = append(choices, registry.Url) | ||
| if selector == "" || registry.Id == selector || registry.Url == selector { | ||
| matches = append(matches, registry) | ||
| } | ||
| } | ||
| if len(matches) == 1 { | ||
| return &matches[0], nil | ||
| } | ||
| if len(matches) == 0 { | ||
| return nil, fmt.Errorf("registry %q is not a ready registry for this project; choose --registry from: %s", selector, strings.Join(choices, ", ")) | ||
| } | ||
| return nil, fmt.Errorf("multiple ready registries found; choose the build source with --registry <url-or-id>: %s (the image is uploaded to all ready registries)", strings.Join(choices, ", ")) | ||
| } |
There was a problem hiding this comment.
The current implementation of selectBuildRegistry creates a temporary slice matches and copies models.ContainerRegistry structs into it. It then returns a pointer to the first element of this temporary slice (&matches[0]). In Go, returning a pointer to an element of a temporary slice causes the slice and its elements to escape to the heap, and copying the structs is slightly inefficient.
We can optimize this by keeping track of the index of the matched registry and the match count, avoiding the slice allocation and struct copying entirely.
func selectBuildRegistry(registries []models.ContainerRegistry, selector string) (*models.ContainerRegistry, error) {
if len(registries) == 0 {
return nil, fmt.Errorf("no registry connections found")
}
var matchIdx = -1
var matchCount = 0
var choices []string
for i, registry := range registries {
choices = append(choices, registry.Url)
if selector == "" || registry.Id == selector || registry.Url == selector {
matchIdx = i
matchCount++
}
}
if matchCount == 1 {
return ®istries[matchIdx], nil
}
if matchCount == 0 {
return nil, fmt.Errorf("registry %q is not a ready registry for this project; choose --registry from: %s", selector, strings.Join(choices, ", "))
}
return nil, fmt.Errorf("multiple ready registries found; choose the build source with --registry <url-or-id>: %s (the image is uploaded to all ready registries)", strings.Join(choices, ", "))
}There was a problem hiding this comment.
Covered by 091c186 and the reply to the earlier comment on this function. The implementation now uses the suggested index/count approach; the three affected Go test packages pass.
There was a problem hiding this comment.
🟡 Changes recommended
Several updated Go test files have import blocks that are not gofmt/goimports ordered, which can break formatting/lint expectations and should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an explicit --registry <url-or-id> selector to control which connected container registry is recorded as the build’s source image URL (while still uploading to all ready registries), improving determinism when multiple registries are configured.
Changes:
- Introduces
selectBuildRegistryto choose the recorded build source registry by URL/ID and require explicit selection when multiple registries exist. - Wires a new
--registryflag intohx buildandhx deploy, and uses it during build artifact recording. - Adds unit tests for registry selection logic and for presence of the new CLI flags.
File summaries
| File | Description |
|---|---|
| internal/build/service.go | Implements registry selection and records the pushed URL from the selected source registry. |
| internal/build/service_test.go | Adds table-driven tests for registry selection behavior. |
| cmd/build/build.go | Adds --registry flag to hx build. |
| cmd/build/build_test.go | Adds a test asserting the --registry flag exists. |
| cmd/deploy/deploy.go | Adds --registry flag to hx deploy. |
| cmd/deploy/deploy_test.go | Adds a test asserting the --registry flag exists. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The selection behavior is implemented cleanly, wired through build and deploy entrypoints, and is backed by targeted unit tests for the new branching logic.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
--registry <url-or-id>tohx buildandhx deployto choose which registry's image URL is recorded on the build and used for deployment.Validation
go test ./internal/build ./cmd/build ./cmd/deploypassed using a temporary Go build cache.gofmtandgit diff --checkpassed.Companion UI: https://github.com/Hyphen/hyphen-app/pull/1550