fix(rest): use lowercase phone_home 'post' key for cloud-init YAML property - #4670
fix(rest): use lowercase phone_home 'post' key for cloud-init YAML property#4670SnobbishFish40 wants to merge 3 commits into
Conversation
Summary by CodeRabbit
WalkthroughThe phone-home method constant now uses ChangesPhone-home configuration
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rest-api/api/pkg/api/model/util/util.go (1)
19-19: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a literal contract assertion for the lowercase key.
The constant now matches the Ubuntu autoinstall requirement. However,
rest-api/api/pkg/api/model/util/util_test.golooks up the generated field throughSitePhoneHomePost, so the test does not independently prove that the serialized configuration containspostrather thanPOST. Assert the literal key and verify thatPOSTis absent.🤖 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.go` at line 19, Update the test using SitePhoneHomePost to assert that the serialized configuration contains the literal lowercase key "post", and also verify that the uppercase key "POST" is absent. Keep the existing generated-field validation while making the assertion independent of the constant’s value.
🤖 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.go`:
- Line 19: Update the test using SitePhoneHomePost to assert that the serialized
configuration contains the literal lowercase key "post", and also verify that
the uppercase key "POST" is absent. Keep the existing generated-field validation
while making the assertion independent of the constant’s value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d7ddef6d-85f2-4556-a47b-8c3983c7255f
📒 Files selected for processing (1)
rest-api/api/pkg/api/model/util/util.go
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-08-06 14:42:22 UTC | Commit: 6f55032 |
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
thossain-nv
changed the title
pbreton
changed the title
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
pbreton
left a comment
pbreton
left a comment
There was a problem hiding this comment.
Please add SnobbishFish40#1 (which I should have done in the first place 😀).
…n-test test(rest-api): validate generated phone-home cloud-init schema
There was a problem hiding this comment.
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/model/util/util_test.go`:
- Around line 15-34: Move the schema-validation logic from
TestInsertedPhoneHomeMatchesCloudInitSchema into a named t.Run case within
TestInsertPhoneHomeIntoUserData. Preserve the existing YAML setup,
InsertPhoneHomeIntoUserData call, cloud-init schema compilation, and validation
assertions, then remove the separate top-level test function.
🪄 Autofix
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: de2e5a66-da28-43ab-997e-3a051ce63b89
📒 Files selected for processing (3)
rest-api/api/pkg/api/model/util/testdata/cloud-init-phone-home.schema.jsonrest-api/api/pkg/api/model/util/util_test.gorest-api/go.mod
| func TestInsertedPhoneHomeMatchesCloudInitSchema(t *testing.T) { | ||
| documentRoot := unmarshalDocumentRoot(t, `autoinstall: | ||
| version: 1 | ||
| `) | ||
| require.NoError(t, InsertPhoneHomeIntoUserData(documentRoot, "http://169.254.169.254/phone_home")) | ||
|
|
||
| autoinstallNode := mappingNodeValue(documentRoot, "autoinstall") | ||
| require.NotNil(t, autoinstallNode) | ||
| targetUserDataNode := mappingNodeValue(autoinstallNode, "user-data") | ||
| require.NotNil(t, targetUserDataNode) | ||
|
|
||
| var targetUserData any | ||
| require.NoError(t, targetUserDataNode.Decode(&targetUserData)) | ||
|
|
||
| compiler := jsonschema.NewCompiler() | ||
| compiler.AssertFormat() | ||
| schema, err := compiler.Compile("testdata/cloud-init-phone-home.schema.json") | ||
| require.NoError(t, err) | ||
| require.NoError(t, schema.Validate(targetUserData)) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move this scenario into the existing table-driven test.
TestInsertedPhoneHomeMatchesCloudInitSchema adds a second top-level test function. Add this validation as a named t.Run case in TestInsertPhoneHomeIntoUserData instead.
As per coding guidelines, rest-api/**/*_test.go requires one top-level Test... function and named table-driven t.Run subtests.
🤖 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 15 - 34, Move the
schema-validation logic from TestInsertedPhoneHomeMatchesCloudInitSchema into a
named t.Run case within TestInsertPhoneHomeIntoUserData. Preserve the existing
YAML setup, InsertPhoneHomeIntoUserData call, cloud-init schema compilation, and
validation assertions, then remove the separate top-level test function.
Source: Coding guidelines
3 participants
When phone-home is enabled for an Ubuntu autoinstall OS, NICo injects the callback configuration into autoinstall.user-data using the uppercase property POST. Ubuntu Subsequently validates this nested cloud-config and rejects POST because the supported property is lowercase post. Installation stops at a fatal-error prompt and the instance remains Provisioning indefinitely.
This regression is exposed by infra-controller PR #3859, which moved NICo's existing phone-home block from the cloud-config document root into the target OS's nested autoinstall.user-data. Uppercase POST had previously worked accidentally at the document root as cloudinit falls back to "post: all" (post_list = ph_cfg.get("post", "all")). Moving it into the strictly validated nested mapping made it crash.
Related issues
Ref: 6569456
Type of Change
Breaking Changes
Testing
No testing was possible without dev environment.