feat(chief): support sub-path hosting via base_url - #211
Conversation
atqamz
left a comment
There was a problem hiding this comment.
Workflow note
This is a follow-up to #210, which was closed and re-opened as a new PR. For future iterations: push additional commits to the same branch (fix/allow-base-url-chief) instead of closing and opening a new PR. This keeps all review history in one thread, prior comments stay anchored to their lines, and reviewers don't need to cross-reference two PRs to follow the conversation.
Also renamed the PR title from Feat/issue 161 base url to feat(chief): support sub-path hosting via base_url for better readability in the PR list.
Prior review items — all addressed
All 9 items from the #210 review (2 blockers, 3 design, 4 nits) have been correctly addressed. Good work.
| # | Item | Status |
|---|---|---|
| B1 | utils/config.yaml scope creep |
Fixed — only 2 commented lines added |
| B2 | Sub-path routing breaks workers | Fixed — dual-mux: inner mux at both / and BaseURL/ |
| D3 | Notification URL used internal Address |
Fixed — new public_url field with Address fallback |
| D4 | Address not normalized |
Fixed — normalizeChiefConfig trims all three fields |
| D5 | Thin BaseURL validation |
Fixed — regex validator ^/[A-Za-z0-9_\-/]*$ |
| N6 | Test swallowed validator error | Fixed — full valid config in tests + direct normalizeChiefConfig unit test |
| N7 | Redundant normalization branch | Fixed — collapsed to suggested form |
| N8 | Silent log URL omission | Fixed — log.Println warning added |
| N9 | Dashboard links not prefix-aware | Fixed — baseurl template helper introduced |
New issues
A — public_url + base_url double-prefix footgun (blocker)
In cmd/builder/builder.go, cmd/iso/iso.go, cmd/repo/repo.go:
logBaseURL := irgshConfig.Chief.PublicURL
if logBaseURL == "" {
logBaseURL = irgshConfig.Chief.Address
}
logBaseURL += irgshConfig.Chief.BaseURLAn operator who sets public_url: 'https://domain.com/irgsh' (the full externally-reachable URL, which is the natural thing to write) AND base_url: '/irgsh' gets:
https://domain.com/irgsh/irgsh/logs/...
The current config comments don't hint at this constraint. Pick one of:
- Document the convention:
public_urlis the origin only (no sub-path);base_urlis appended automatically. Add a comment inutils/config.yamlnext to thepublic_urlexample:# origin only — base_url is appended automatically. - Drop the append when
public_urlis set: If the operator providedpublic_url, assume it's already the complete external URL and don't appendBaseURL.
Either option works; option 1 is a one-line doc fix, option 2 changes the logic. Without one of these, operators deploying behind a reverse proxy will get broken notification links that are hard to diagnose.
B — RegisterValidation error swallowed (nit)
internal/config/config.go:
validate.RegisterValidation("baseurl", func(fl validator.FieldLevel) bool { ... })
return validate.Struct(cfg)RegisterValidation returns an error if the tag name is empty or the function is nil. The return value is ignored. One line:
if err := validate.RegisterValidation("baseurl", ...); err != nil {
return err
}C — Regex compiled on every config load (nit)
regexp.MustCompile runs inside the closure on every call to applyDefaults. Lift to package level:
var baseURLRegex = regexp.MustCompile(`^/[A-Za-z0-9_\-/]*$`)Reference from the closure instead of compiling each time.
D — Dual-mux: /api/v1/* accessible at both paths — add a comment (nit)
The dual-mux means /irgsh/api/v1/version also responds (worker routes reachable via prefix too). This is intentional for Option 3, but a future reader will see the double-registration and wonder if it's a bug. One comment in setupRoutes:
// rootMux forwards prefixed browser traffic to mux while also keeping mux
// reachable at root so workers can call /api/v1/* without config changes.E — Stray blank-line removals (nit)
The diff removes the blank line between sendBuildNotification/sendISONotification/sendRepoNotification and the following Build/BuildISO/Repo function in all three worker files. Unrelated to this feature; restore them.
F — TestBaseURLValidation may not fire the baseurl validator (nit)
The minimal IrgshConfig in the test omits Repo, ISO, Storage, Monitoring. If those structs have validate:"required" fields, applyDefaults may return a different validation error before reaching the baseurl check — valid BaseURL cases would still pass (error for wrong reason), but invalid cases might report the wrong field. CI is green so it works today.
Suggest adding one assertion that a bad BaseURL produces an error mentioning BaseURL or baseurl, to confirm the right validator fires.
All prior blockers resolved — the core approach is solid. Issue A is the only item blocking merge; B–F can be cleaned up in the same PR.
|
I have addressed all the feedback items (A-F) from the latest review. Thank you for the thorough review! |
ef94a68 to
268004a
Compare
268004a to
009a922
Compare
|
Thanks again for the thorough review. All six new items (A–F) are addressed in A — B — C — Regex recompiled per load — Lifted to package-level D — Dual-mux needs a comment — Added at E — Stray blank-line removals — Restored before F — Test may not fire the Verification: |
|
Hi @ssFari, thanks for contributing. Can we wrap this within irgshConfig struct? So we don't have to put conditional check here and there. |
|
Thanks @herpiko, good call. Done in The func sendBuildNotification(taskUUID, status string, jobInfo notification.JobNotificationInfo) {
notification.SendJobNotification(
irgshConfig.Notification.WebhookURL,
irgshConfig.FullBaseURL,
"Build",
taskUUID,
status,
jobInfo,
)
}Same change applied to |
Compute the externally-reachable log base URL once at config load time into IrgshConfig.FullBaseURL, instead of repeating the public_url/base_url conditional in builder, iso, and repo notification senders.
7a9a3fc to
607f66b
Compare
feat: allow irgsh-chief to run under a sub-path with base_url (v2)
Description Thank you for the detailed feedback on the previous iteration. I have refactored the implementation to address all blockers and design concerns. This version ensures full backward compatibility with workers while providing a robust sub-path support for the dashboard.
Addresssed Feedback & Improvements
Clean utils/config.yaml (Blocker Add clone task #1)
Dual-Mux Routing for Worker Compatibility (Blocker CI/CD #2)
Dedicated PublicURL & Proper Normalization (Design SQL Schema #3 & Fix urfave cli #4)
URLs.
double-slash issues (//logs/...).
Strict BaseURL Validation (Design Working chainned signature through endpoint. #5)
Enhanced Testing & Refactoring (Nit Minimal complete builder worker #6 & Repo initialization. Working package injection.=[ #7)
unrelated errors.
Visibility & Maintainability (Nit irgsh-cli is now part of irgsh-go #8 & Improve docs #9)
consistency across the UI.
Verification Results
Fixes