refactor(rest-api): Use our generic cdb.GetPtr instead of per-type GetXPtr helpers by chet · Pull Request #2171 · NVIDIA/infra-controller

chet · 2026-06-03T21:27:16Z

Description

@bcavnvidia pointed out that after cdb.Ptr got introduced as a generic helper in NVIDIA/infra-controller-rest#569 (which I probably should have just named cdb.GetPtr), we could actually get rid of all of our type-specific helpers and just lean on this across the board.

To summarize:

cutil.GetPtr (in common/pkg/util) is now the one helper for any value type (renamed from cdb.Ptr and moved out of db/pkg/db -- thanks to @thossain-nv for that suggestion).
The per-type helpers (GetStrPtr, GetIntPtr, GetBoolPtr, GetUUIDPtr, GetTimePtr) go away.

But, cdb.GetTypedStrPtr stays, because it converts a typed-string value (e.g. MachineCapabilityType) to *string, which GetPtr can't do generically (i.e. it'd return *MachineCapabilityType).

Per-type tests collapsed into a single TestGetPtr with type subtests, alongside its converter tests.

Signed-off-by: Chet Nichols III chetn@nvidia.com

Type of Change

Add - New feature or capability
Change - Changes in existing functionality
Fix - Bug fixes
Remove - Removed features or deprecated functionality
Internal - Internal changes (refactoring, tests, docs, etc.)

Related Issues (Optional)

Breaking Changes

This PR contains breaking changes

Testing

Unit tests added/updated
Integration tests added/updated
Manual testing performed
No testing required (docs, internal refactor, etc.)

Additional Notes

coderabbitai · 2026-06-03T21:27:23Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 04800eea-6585-4d48-b6f9-7be7e41e0f38

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

github-actions · 2026-06-03T21:29:30Z

🔐 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-06-03 21:29:30 UTC | Commit: 5958b51}

chet · 2026-06-03T21:29:46Z

Hey @coderabbitai! There's a .coderabbit.yaml in rest-api/ for you to use (or maybe it's in the repo root now)? Care to take a peek? You helped provide guidance for some of the original work around cbd.GetPtr (what was cbd.Ptr) based on the original PR I did to introduce it, so I think you should be pleased with this iterative effort around it. Thank you!

coderabbitai · 2026-06-03T21:30:57Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check for .coderabbit.yaml in rest-api/ and repo root
echo "=== rest-api/.coderabbit.yaml ==="
cat rest-api/.coderabbit.yaml 2>/dev/null || echo "(not found)"

echo ""
echo "=== .coderabbit.yaml (root) ==="
cat .coderabbit.yaml 2>/dev/null || echo "(not found)"

echo ""
echo "=== rest-api/AGENTS.md ==="
cat rest-api/AGENTS.md 2>/dev/null || echo "(not found)"

Length of output: 30783

🏁 Script executed:

#!/bin/bash
# Fetch the PR diff for `#2171`
gh pr view 2171 --json title,body,files 2>/dev/null | jq '.'
gh pr diff 2171 2>/dev/null | head -500

Length of output: 56820

@chet — good to hear from you, and thank you for the kind words. I have reviewed the .coderabbit.yaml (it is at the repo root rather than rest-api/; I will update my notes accordingly), rest-api/AGENTS.md, and the PR diff.

Configuration

The root .coderabbit.yaml is well-structured:

auto_review: enabled: false means I will only engage when explicitly invoked — the model that suits a repo of this scale.
Path instructions are correctly scoped: Go, Proto, OpenAPI, shell, Markdown, and the auto-generated sdk/standard carve-out are all accounted for.
The knowledge_base.code_guidelines file patterns now pick up AGENTS.md and rest-api/AGENTS.md, which is the right move; the conventions documented there are substantial and I will use them in future reviews.

PR assessment

This is a clean, production-grade refactor. Specific observations:

Uniform generic surface. cdb.GetPtr[T] is a strict improvement: one entry point, zero per-type maintenance, and the Go type system enforces correctness at every call site. The diff is large in file count but trivially mechanical — no logic changes are introduced.
Correct retention of cdb.GetTypedStrPtr. This is the right call. GetPtr on a MachineCapabilityType would yield *MachineCapabilityType, not *string. Per the AGENTS.md named-types convention, the typed-string enum owns its conversion behavior; GetTypedStrPtr is the narrow bridge from that domain type to a wire *string. Collapsing it into the generic would violate that boundary.
Test consolidation. Collapsing per-type TestGetXPtr functions into a single TestGetPtr with typed subtests is idiomatic Go test structure. It reduces boilerplate while keeping coverage parity.
db.GetPtr(0) at expectedTotal. This uses a different package alias (db) than the rest of the file (cdb). Assuming both resolve to the same function post-rename, this is a non-issue — but worth confirming that the db alias is intentional and not a stale import alias from before the rename.

No blocking concerns. The API is simpler, the intent is clearer, and the AGENTS.md conventions are respected throughout. Well done to you and @bcavnvidia for the follow-through.

🐇 ✨

✏️ Learnings added

Learnt from: chet
URL: https://github.com/NVIDIA/infra-controller/pull/2171

Timestamp: 2026-06-03T21:30:56.488Z
Learning: In the NVIDIA/infra-controller repository, the `.coderabbit.yaml` configuration file is located at the **repo root** (not at `rest-api/.coderabbit.yaml`). The `rest-api/AGENTS.md` file contains detailed coding conventions for the Go codebase and should be consulted for reviews involving `rest-api/` code. `auto_review` is disabled (`enabled: false`), so CodeRabbit reviews are triggered only on explicit invocation.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used

Learnt from: chet
Repo: NVIDIA/infra-controller PR: 0
File: :0-0
Timestamp: 2026-06-02T19:13:04.224Z
Learning: For the NVIDIA/infra-controller repository, review guidance for rest-api changes is available in `rest-api/.coderabbit.yaml` and `rest-api/AGENTS.md`; future reviews involving `rest-api` code should consult those files.

Learnt from: chet
Repo: NVIDIA/infra-controller PR: 0
File: :0-0
Timestamp: 2026-06-02T19:06:09.917Z
Learning: In `infra-controller-rest` (Go), named types own their proto behavior — `map`, `slice`, or other primitive composites representing domain concepts get named types with methods, not free functions. Examples: `db/pkg/db/model.Labels` (`type Labels map[string]string` with `ToProto`/`FromProto`), `MachineCapabilityType`/`MachineCapabilityDeviceType` (`type X string` with typed proto enum round-trip). Free functions like `LabelsToProto(m map[string]string)` are an anti-pattern.

thossain-nv · 2026-06-03T21:35:23Z

Hi @chet, we could take this opportunity to move the GetPtr function to https://github.com/NVIDIA/infra-controller/blob/main/rest-api/common/pkg/util/converter.go!

chet · 2026-06-03T21:36:26Z

Hi @chet, we could take this opportunity to move the GetPtr function to https://github.com/NVIDIA/infra-controller/blob/main/rest-api/common/pkg/util/converter.go!

@thossain-nv Genius!! Moving!

github-actions · 2026-06-03T21:43:03Z

🔍 Container Scan Summary

Service	Total	Critical	High	Medium	Low	Other
nico-flow	66	4	34	18	2	8
nico-nsm	82	2	28	43	9	0
nico-psm	67	4	35	18	2	8
nico-rest-api	100	6	53	30	3	8
nico-rest-cert-manager	65	4	34	18	1	8
nico-rest-db	66	4	34	18	2	8
nico-rest-site-agent	65	4	34	18	1	8
nico-rest-site-manager	65	4	34	18	1	8
nico-rest-workflow	67	4	35	18	2	8
TOTAL	643	36	321	199	23	64

Per-CVE detail lives in the per-service grype-* artifacts (JSON + SARIF). Severity counts only — no CVE IDs published here.

@bcavnvidia

…tXPtr helpers @bcavnvidia pointed out that after `cdb.Ptr` got introduced as a generic helper in NVIDIA/infra-controller-rest#569 (which I probably should have just named `cdb.GetPtr`), we could actually get rid of all of our type-specific helpers and just lean on this across the board. To summarize: - `cutil.GetPtr` (in `common/pkg/util`) is now the one helper for any value type (renamed from `cdb.Ptr` and moved out of `db/pkg/db` -- thanks to @thossain-nv for that suggestion). - The per-type helpers (`GetStrPtr`, `GetIntPtr`, `GetBoolPtr`, `GetUUIDPtr`, `GetTimePtr`) go away. But, `cdb.GetTypedStrPtr` stays, because it converts a typed-string value (e.g. `MachineCapabilityType`) to `*string`, which `GetPtr` can't do generically (i.e. it'd return `*MachineCapabilityType`). Per-type tests collapsed into a single `TestGetPtr` with type subtests, alongside its sibling converter tests. Signed-off-by: Chet Nichols III <chetn@nvidia.com>

chet · 2026-06-03T21:57:46Z

Hi @chet, we could take this opportunity to move the GetPtr function to https://github.com/NVIDIA/infra-controller/blob/main/rest-api/common/pkg/util/converter.go!

@thossain-nv Genius!! Moving!

@thossain-nv Done!

thossain-nv

Thank you for moving it to common!

chet requested a review from a team as a code owner June 3, 2026 21:27

chet force-pushed the rest-use-get-ptr branch from 5958b51 to 0fa594b Compare June 3, 2026 21:47

chet force-pushed the rest-use-get-ptr branch from 0fa594b to dcca67e Compare June 3, 2026 21:47

thossain-nv approved these changes Jun 3, 2026

View reviewed changes

chet merged commit 8b4188b into NVIDIA:main Jun 3, 2026
89 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(rest-api): Use our generic cdb.GetPtr instead of per-type GetXPtr helpers#2171

refactor(rest-api): Use our generic cdb.GetPtr instead of per-type GetXPtr helpers#2171
chet merged 1 commit into
NVIDIA:mainfrom
chet:rest-use-get-ptr

chet commented Jun 3, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jun 3, 2026 •

edited

Loading

Review skipped

Uh oh!

github-actions Bot commented Jun 3, 2026

Uh oh!

chet commented Jun 3, 2026

Uh oh!

coderabbitai Bot commented Jun 3, 2026

Uh oh!

thossain-nv commented Jun 3, 2026

Uh oh!

chet commented Jun 3, 2026

Uh oh!

github-actions Bot commented Jun 3, 2026 •

edited

Loading

Uh oh!

chet commented Jun 3, 2026

Uh oh!

thossain-nv left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

chet commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Related Issues (Optional)

Breaking Changes

Testing

Additional Notes

Uh oh!

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

Uh oh!

Review skipped

Uh oh!

github-actions Bot commented Jun 3, 2026

🔐 TruffleHog Secret Scan

Uh oh!

chet commented Jun 3, 2026

Uh oh!

coderabbitai Bot commented Jun 3, 2026

Uh oh!

thossain-nv commented Jun 3, 2026

Uh oh!

chet commented Jun 3, 2026

Uh oh!

github-actions Bot commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Container Scan Summary

Uh oh!

chet commented Jun 3, 2026

Uh oh!

thossain-nv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chet commented Jun 3, 2026 •

edited

Loading

coderabbitai Bot commented Jun 3, 2026 •

edited

Loading

github-actions Bot commented Jun 3, 2026 •

edited

Loading