useLongNames mode includes .local in its allowed TLDs. Also turned on useLongNames - #2474
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates FQDN domain stripping to recognize ChangesHost FQDN and Render Host Insertion Changes
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cuebot/src/main/java/com/imageworks/spcue/dao/postgres/HostDaoJdbc.java (1)
667-674: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression test for
.localdomain preservation.This regex change is the core fix described in the PR objective (preserving
.localhostnames), but no test in the provided context exercisesgetHostNameFromFQDN/insertRenderHostwith a.localFQDN underuseLongNames=true. Consider adding a case similar totestInsertHostFQDN1inHostDaoTests.javaasserting that a name likemyhost.localis preserved as-is.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cuebot/src/main/java/com/imageworks/spcue/dao/postgres/HostDaoJdbc.java` around lines 667 - 674, Add a regression test covering `.local` hostname preservation when `useLongNames` is enabled. Update the existing host DAO test suite around `HostDaoTests` to exercise `insertRenderHost` and/or `getHostNameFromFQDN` with a `.local` FQDN similar to `myhost.local`, and assert the stored/resolved host name remains unchanged instead of being stripped. Use the existing test pattern from `testInsertHostFQDN1` and keep the new assertion focused on `HostDaoJdbc`’s FQDN parsing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cuebot/src/main/java/com/imageworks/spcue/service/HostManagerService.java`:
- Line 166: The hardcoded true passed to hostDao.insertRenderHost in
HostManagerService changes hostname derivation for all FQDNs, not just .local.
Update the HostManagerService flow to avoid forcing useLongNames globally;
instead make this behavior configurable or limit it to the Bonjour/Avahi .local
case, and keep the existing short-name behavior for other domains. Verify the
change against HostDaoJdbc.getHostNameFromFQDN and the host insert path so
findDispatchHost and related hostname-based lookups continue to work for
internal domains.
---
Nitpick comments:
In `@cuebot/src/main/java/com/imageworks/spcue/dao/postgres/HostDaoJdbc.java`:
- Around line 667-674: Add a regression test covering `.local` hostname
preservation when `useLongNames` is enabled. Update the existing host DAO test
suite around `HostDaoTests` to exercise `insertRenderHost` and/or
`getHostNameFromFQDN` with a `.local` FQDN similar to `myhost.local`, and assert
the stored/resolved host name remains unchanged instead of being stripped. Use
the existing test pattern from `testInsertHostFQDN1` and keep the new assertion
focused on `HostDaoJdbc`’s FQDN parsing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28602e10-5843-4f13-898a-ff3127e31034
📒 Files selected for processing (2)
cuebot/src/main/java/com/imageworks/spcue/dao/postgres/HostDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/service/HostManagerService.java
| public DispatchHost createHost(RenderHost rhost, AllocationEntity alloc) { | ||
|
|
||
| hostDao.insertRenderHost(rhost, alloc, false); | ||
| hostDao.insertRenderHost(rhost, alloc, true); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Hardcoding useLongNames=true changes hostname derivation for all hosts, not just .local.
Per getHostNameFromFQDN in HostDaoJdbc.java, useLongNames=false unconditionally strips everything after the first ., whereas useLongNames=true only strips the domain suffix if it matches a hardcoded TLD pattern (com|biz|edu|info|net|org|cn|de|eu|nl|local). Any FQDN with a domain suffix outside this list (e.g. internal domains like .corp, .lan, .internal, or other ccTLDs) will now be stored with the full FQDN as the host name instead of the short hostname, since the regex won't match and the fallback keeps hostName = fqdn unchanged.
This affects every deployment on upgrade, not only Bonjour/Avahi .local setups as intended by the PR objective, and could disrupt downstream hostname-based lookups/dispatching (findDispatchHost, tagging, RQD name matching) for installations using unlisted internal domains.
Consider making this configurable (e.g., via a property) rather than hardcoding true, or extending the pattern/logic to explicitly special-case .local while preserving prior stripping behavior for other domains.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cuebot/src/main/java/com/imageworks/spcue/service/HostManagerService.java` at
line 166, The hardcoded true passed to hostDao.insertRenderHost in
HostManagerService changes hostname derivation for all FQDNs, not just .local.
Update the HostManagerService flow to avoid forcing useLongNames globally;
instead make this behavior configurable or limit it to the Bonjour/Avahi .local
case, and keep the existing short-name behavior for other domains. Verify the
change against HostDaoJdbc.getHostNameFromFQDN and the host insert path so
findDispatchHost and related hostname-based lookups continue to work for
internal domains.
dd23436 to
1e7d8e7
Compare
…ts allowed TLDs. Also turned on useLongNames.
35f601a to
b42e4f0
Compare
| Pattern domainPattern = Pattern | ||
| .compile(".*(\\.(.*)\\.(co(m|.[a-z]{2})|biz|edu|info|net|org|cn|de|eu|nl))$"); |
There was a problem hiding this comment.
I don't think this is related to avahi host.local hostnames, but only hostnames like host.domain.local
2 participants
Summarize your change.
For my setup I've ended up just using mostly bonjour/avahi for name resolution - i.e. .local
I could never get the RQD hosts to properly show up with the .local, it was always stripped off - even using the OVERRIDE_HOSTNAME setting in my rqd.conf.
Turns out not only was cuebot stripping off .local because it's not in its list of valid TLDs, but also
useLongNamesis hardcoded tofalse.I've allowed .local and hardcoded
useLongNamestotrue.Summary by CodeRabbit
.localsuffixes are stripped and interpreted correctly.