Rename hosts name template gitops#48787
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 38806-macos-iosipados-rename-hosts #48787 +/- ##
=====================================================================
Coverage ? 68.03%
=====================================================================
Files ? 3683
Lines ? 234281
Branches ? 12419
=====================================================================
Hits ? 159399
Misses ? 60533
Partials ? 14349
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
juan-fdz-hawa
force-pushed
the
48623-rename-hosts-name-template-endpoint
branch
from
049d8eb to
119918f
Compare
juan-fdz-hawa
force-pushed
the
48623-rename-hosts-name-template-gitops
branch
from
183821a to
02ae944
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughChangesThis PR extends Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as ee/server/service
participant Validator as fleet.ValidateHostNameTemplate
participant Enforcer as applyHostNameTemplateChange
participant Store
Client->>Service: PATCH team or apply spec (MDM.HostNameTemplate)
Service->>Validator: validate template value
Validator-->>Service: validation result
alt valid
Service->>Store: save team config with updated template
Service->>Enforcer: apply enforcement if template changed
else invalid
Service-->>Client: return validation error
end
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (2)
ee/server/service/teams.go (2)
1526-1534: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHostNameTemplate validation error short-circuits instead of aggregating with other spec errors.
createTeamFromSpeccollects most spec validation failures intoinvalidand reports them together (seefleet.ValidateMDMProfileSpecs,host_expiry_settings, etc. at Lines 1539-1594), which is friendlier for GitOps dry-run users fixing multiple issues at once. The new HostNameTemplate check at Lines 1528-1533 instead returns immediately on error, so if the spec has both an invalid template and another invalid field, only the template error surfaces.Consider moving the
invalid := &fleet.InvalidArgumentError{}declaration above this block and usinginvalid.Append("mdm.host_name_template", err.Error())(or embed the InvalidArgumentError) instead of an early return, for consistency with the rest of this function.♻️ Sketch of aggregation approach
- nameTemplate := spec.MDM.HostNameTemplate.Value - if nameTemplate != "" { - validated, err := fleet.ValidateHostNameTemplate(nameTemplate) - if err != nil { - return nil, ctxerr.Wrap(ctx, err) - } - nameTemplate = validated - } - invalid := &fleet.InvalidArgumentError{} + nameTemplate := spec.MDM.HostNameTemplate.Value + if nameTemplate != "" { + validated, err := fleet.ValidateHostNameTemplate(nameTemplate) + if err != nil { + invalid.Append("mdm.host_name_template", err.Error()) + } else { + nameTemplate = validated + } + } if enableDiskEncryption && svc.config.Server.PrivateKey == "" {Also applies to: 1616-1616, 1677-1682
🤖 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 `@ee/server/service/teams.go` around lines 1526 - 1534, The HostNameTemplate validation in createTeamFromSpec is returning immediately instead of being added to the same aggregated InvalidArgumentError as the other spec checks. Move the invalid accumulator so it is available before the spec validation blocks, and in the HostNameTemplate branch append the validation failure (for example under the mdm.host_name_template key) rather than calling ctxerr.Wrap and returning early. Keep the existing validation logic in createTeamFromSpec consistent with the other aggregated checks used around ValidateMDMProfileSpecs and host_expiry_settings, and apply the same pattern to the other HostNameTemplate validation sites referenced in the diff.
1810-1823: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame early-return validation gap as
createTeamFromSpec.
editTeamFromSpecalso aggregates most validation failures intoinvalid(declared at Line 1900, used through Line 2020), but the HostNameTemplate check here (Lines 1811-1822) returns immediately on validation error instead of appending toinvalid. This means, e.g., a GitOps apply with both an invalid host name template and an invalid custom setting only reports the first error encountered.Since
invalidis declared later in this function (Line 1900), moving its declaration above this block (or aggregating the error separately) would let both issues surface together for GitOps dry runs.Also applies to: 2089-2094
🤖 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 `@ee/server/service/teams.go` around lines 1810 - 1823, The HostNameTemplate validation in editTeamFromSpec returns immediately on error, which bypasses the function’s existing invalid aggregation. Update editTeamFromSpec so the fleet.ValidateHostNameTemplate failure is added to invalid instead of exiting early, matching the createTeamFromSpec behavior and allowing GitOps dry runs to report multiple issues together. Use the existing invalid collector in editTeamFromSpec and keep the HostNameTemplate assignment logic gated on successful validation; apply the same fix to the duplicate HostNameTemplate path noted around 2089-2094.
🤖 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.
Nitpick comments:
In `@ee/server/service/teams.go`:
- Around line 1526-1534: The HostNameTemplate validation in createTeamFromSpec
is returning immediately instead of being added to the same aggregated
InvalidArgumentError as the other spec checks. Move the invalid accumulator so
it is available before the spec validation blocks, and in the HostNameTemplate
branch append the validation failure (for example under the
mdm.host_name_template key) rather than calling ctxerr.Wrap and returning early.
Keep the existing validation logic in createTeamFromSpec consistent with the
other aggregated checks used around ValidateMDMProfileSpecs and
host_expiry_settings, and apply the same pattern to the other HostNameTemplate
validation sites referenced in the diff.
- Around line 1810-1823: The HostNameTemplate validation in editTeamFromSpec
returns immediately on error, which bypasses the function’s existing invalid
aggregation. Update editTeamFromSpec so the fleet.ValidateHostNameTemplate
failure is added to invalid instead of exiting early, matching the
createTeamFromSpec behavior and allowing GitOps dry runs to report multiple
issues together. Use the existing invalid collector in editTeamFromSpec and keep
the HostNameTemplate assignment logic gated on successful validation; apply the
same fix to the duplicate HostNameTemplate path noted around 2089-2094.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b500532-972c-4ab7-b495-43ec221090f1
📒 Files selected for processing (3)
ee/server/service/teams.goserver/service/apple_mdm_test.goserver/service/integration_enterprise_test.go
juan-fdz-hawa
force-pushed
the
48623-rename-hosts-name-template-gitops
branch
from
02ae944 to
accd784
Compare
juan-fdz-hawa
force-pushed
the
48623-rename-hosts-name-template-gitops
branch
from
accd784 to
0d92176
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Relates to #48623 Plumbs mdm.name_template through PATCH /teams/{id} and team-spec apply (create and edit, which GitOps rides on).
juan-fdz-hawa
force-pushed
the
48623-rename-hosts-name-template-gitops
branch
from
0d92176 to
59cad67
Compare
juan-fdz-hawa
merged commit f278514
into
38806-macos-iosipados-rename-hosts
2 participants
Relates to #48623
Plumbs mdm.name_template through PATCH /teams/{id} and team-spec apply (create and edit, which GitOps rides on).
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Testing
Summary by CodeRabbit
New Features
Bug Fixes