Merged
Conversation
enables full differences between products without requiring full url specification. not sure it is the best path, but it keeps backwards compat with the existing prefix tooling and implementers don't have to deal with use cases where some fields are ignored.
|
Claude finished @stevenolen's task —— View job Reviewing PR #66: add basedomain to each product
Review submitted. Key findings:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Overall, this is a well-structured change that maintains backward compatibility while enabling per-product domain configuration. The tests are comprehensive and cover the key scenarios.
API Changes (api/core/v1beta1/site_types.go)
- ✅ Kubebuilder annotations are correct (
+optional) - ✅ Sensible defaults (empty string falls back to
site.Spec.Domain) - ✅ No breaking changes - backward compatible
- Suggestion: Consider adding hostname format validation via kubebuilder annotations (e.g.,
+kubebuilder:validation:Pattern) to catch invalid domain values early.
Controller Changes (internal/controller/core/site_controller.go)
- ✅ Reconciliation remains idempotent
- ✅ Config flows correctly from Site -> Product
- ✅ The
getEffectiveBaseDomainhelper is clear and simple
Flightdeck (flightdeck/html/home.go)
- ✅ Go templates render correctly with the new logic
- Important:
getEffectiveBaseDomainis duplicated in bothflightdeck/html/home.go:12-17andinternal/controller/core/site_controller.go:92-97. Consider extracting to a shared utility package to avoid duplication.
Tests (internal/controller/core/site_test.go)
- ✅ Comprehensive coverage of key scenarios:
- BaseDomain not set (backward compatibility)
- Single product with BaseDomain
- All products with BaseDomain
- Custom prefix with BaseDomain
- ✅ Verifies
DefaultRSConnectServercorrectly uses Connect's BaseDomain
stevenolen
commented
Feb 3, 2026
Collaborator
Author
There was a problem hiding this comment.
this looks like it should belong to a different pr, it was created with make helm-generate though, which is needed for this pr.
timtalbot
approved these changes
Feb 3, 2026
Contributor
timtalbot
left a comment
timtalbot
left a comment
There was a problem hiding this comment.
Weird that the auth proxy shows up here but to your point if generate is making that change then it's expected elsewhere 👍
ian-flores
added a commit
that referenced
this pull request
Feb 4, 2026
Adds a GitHub Action that validates PR titles follow conventional commit format (feat:, fix:, docs:, etc.). This ensures squash merges produce commits that semantic-release can analyze for version bumps. Required because semantic-release was skipping releases when commits didn't follow the convention (e.g., PR #66's "fix tests" instead of "fix: tests").
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
enables full differences between products without requiring full url specification. not sure it is the best path, but it keeps backwards compat with the existing prefix tooling and implementers don't have to deal with use cases where some fields are ignored.