fix(core): phone_home should NOT be injected in installer user-data but in target OS user-data by pbreton · Pull Request #3859 · NVIDIA/infra-controller

pbreton · 2026-07-22T18:16:32Z

When using an installer to install OS on target, 'phone_home' is added to the user-data of the installer instead of the user-data for the target OS. That makes the installer to callback and instances to report Ready/BootCompleted before the OS is actually booted/reachable.

Type of Change

Fix - Bug fixes

Breaking Changes

This PR contains breaking changes

Testing

Unit tests added/updated

…ut in target OS user-data Signed-off-by: Patrice Breton <pbreton@nvidia.com>

coderabbitai · 2026-07-22T18:16:52Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d8c119dd-612f-4465-8fbc-cccb64f1cbd4

📥 Commits

Reviewing files that changed from the base of the PR and between 42f1ae7 and fbf9129.

📒 Files selected for processing (2)

rest-api/api/pkg/api/handler/instance_test.go
rest-api/api/pkg/api/model/util/util.go

Summary by CodeRabbit

Bug Fixes
- Phone-home settings are now correctly added and removed both at the document root and under autoinstall.user-data.
- Removal can now delete all phone-home entries or only those matching a specified URL.
- Invalid autoinstall.user-data structures now return a clear error instead of being modified incorrectly.
Tests
- Added unit tests covering insertion, selective removal, nested configurations, and invalid YAML structures.
- Updated instance creation tests to validate phone-home presence by parsing cloud-init YAML rather than checking raw text.

Walkthrough

Phone-home YAML handling now supports root-level and autoinstall.user-data mappings. Removal handles all or URL-specific blocks, while insertion selects or creates the appropriate mapping and validates YAML node types. Tests cover utility behavior, error cases, and parsed instance output.

Changes

Phone-home YAML handling

Layer / File(s)	Summary
Remove phone-home blocks `rest-api/api/pkg/api/model/util/util.go`, `rest-api/api/pkg/api/model/util/util_test.go`	Removal now operates on root and nested mappings, supports removing all or URL-matching blocks, and safely adjusts iteration after splicing nodes.
Select insertion mapping `rest-api/api/pkg/api/model/util/util.go`, `rest-api/api/pkg/api/model/util/util_test.go`	Insertion targets the root or `autoinstall.user-data`, creates missing user-data mappings, rejects invalid mapping nodes, and verifies generated URL and post values.
Validate parsed instance output `rest-api/api/pkg/api/handler/instance_test.go`	Instance tests detect phone-home blocks at the root or under `autoinstall.user-data` after YAML parsing.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly states the main fix: moving phone_home injection from installer user-data to target OS user-data.
Description check	✅ Passed	The description matches the change and explains the installer-vs-target OS phone_home bug and the test updates.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

🧹 Nitpick comments (1)

rest-api/api/pkg/api/model/util/util_test.go (1)
45-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover non-mapping autoinstall.

The table covers invalid autoinstall.user-data but not the new autoinstall mapping guard. Add an autoinstall: invalid error case to lock down that branch.
Proposed test case
+		{
+			name:     "rejects non-mapping autoinstall",
+			userData: "autoinstall: invalid\n",
+			wantErr:  true,
+		},
🤖 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 `@rest-api/api/pkg/api/model/util/util_test.go` around lines 45 - 53, Add a
table-driven test case alongside the existing autoinstall validation cases in
the relevant test table, using scalar `autoinstall: invalid` input and asserting
the expected error result to cover the non-mapping autoinstall guard. Keep the
existing nested user-data case unchanged.
Source: Coding guidelines

🤖 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 `@rest-api/api/pkg/api/model/util/util_test.go`:
- Around line 45-53: Add a table-driven test case alongside the existing
autoinstall validation cases in the relevant test table, using scalar
`autoinstall: invalid` input and asserting the expected error result to cover
the non-mapping autoinstall guard. Keep the existing nested user-data case
unchanged.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7bb2001f-bc47-458f-867a-f3c86ea50818

📥 Commits

Reviewing files that changed from the base of the PR and between 86b8b5e and a8611c0.

📒 Files selected for processing (2)

rest-api/api/pkg/api/model/util/util.go
rest-api/api/pkg/api/model/util/util_test.go

github-actions · 2026-07-22T18:22:36Z

🔐 TruffleHog Secret Scan

✅ No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

_{🕐 Last updated: 2026-07-22 18:22:36 UTC | Commit: a8611c0}

Signed-off-by: Patrice Breton <pbreton@nvidia.com>

coderabbitai

Actionable comments posted: 1

🤖 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 `@rest-api/api/pkg/api/handler/instance_test.go`:
- Around line 58-73: Update userDataHasPhoneHome to identify the exact
phone-home location and reject configurations where it appears at the wrong
level or in both root and nested user-data. In
rest-api/api/pkg/api/handler/instance_test.go lines 3898-3906, assert root
placement for root-only user-data and nested placement for autoinstall
user-data; at lines 3919-3919, apply the same location-specific assertion to
inherited OS user-data.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c8dc4cc3-44ff-4794-ba54-f33862caf692

📥 Commits

Reviewing files that changed from the base of the PR and between a8611c0 and 42f1ae7.

📒 Files selected for processing (1)

rest-api/api/pkg/api/handler/instance_test.go

Signed-off-by: Patrice Breton <pbreton@nvidia.com>

fix(core): phone_home should NOT be injected in installer user-data b…

a8611c0

…ut in target OS user-data Signed-off-by: Patrice Breton <pbreton@nvidia.com>

pbreton requested a review from a team as a code owner July 22, 2026 18:16

coderabbitai Bot reviewed Jul 22, 2026

View reviewed changes

chet approved these changes Jul 22, 2026

View reviewed changes

fix test

42f1ae7

Signed-off-by: Patrice Breton <pbreton@nvidia.com>

coderabbitai Bot reviewed Jul 22, 2026

View reviewed changes

Comment thread rest-api/api/pkg/api/handler/instance_test.go Outdated

hwadekar-nv reviewed Jul 22, 2026

View reviewed changes

Comment thread rest-api/api/pkg/api/model/util/util.go Outdated

hwadekar-nv approved these changes Jul 22, 2026

View reviewed changes

inline single-use function

fbf9129

Signed-off-by: Patrice Breton <pbreton@nvidia.com>

pbreton enabled auto-merge (squash) July 22, 2026 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(core): phone_home should NOT be injected in installer user-data but in target OS user-data#3859

fix(core): phone_home should NOT be injected in installer user-data but in target OS user-data#3859
pbreton wants to merge 3 commits into
NVIDIA:mainfrom
pbreton:fix/NV6404876

pbreton commented Jul 22, 2026

Uh oh!

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

pbreton commented Jul 22, 2026

Type of Change

Breaking Changes

Testing

Uh oh!

coderabbitai Bot commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 22, 2026

🔐 TruffleHog Secret Scan

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading