Skip to content

chore(rest-api): Apply our new FromProto/ToProto modeling to Machine#2139

Open
chet wants to merge 1 commit into
NVIDIA:mainfrom
chet:proto-machine-receivers
Open

chore(rest-api): Apply our new FromProto/ToProto modeling to Machine#2139
chet wants to merge 1 commit into
NVIDIA:mainfrom
chet:proto-machine-receivers

Conversation

@chet
Copy link
Copy Markdown
Contributor

@chet chet commented Jun 2, 2026

Description

Brings two inline workflow-request builders in the Machine handler in line with the proto-modeling changes. Both flows have no API request body -- Maintenance toggles a state and Metadata-update syncs labels -- so the methods live on the entity rather than on an API request type.

Primary callouts are:

  • Maintenance: Machine.ToMaintenanceRequestProto(operation, reference).
  • Metadata-update: Machine.ToMetadataUpdateRequestProto(labels) (reads the current site-metadata Name from stored metadata, falling back to m.ID).
  • Handler simplified to one-liners at both sites.

Tests added!

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

Brings two inline workflow-request builders in the `Machine` handler in line with the proto-modeling changes. Both flows have no API request body -- Maintenance toggles a state and Metadata-update syncs labels -- so the methods live on the entity rather than on an API request type.

Primary callouts are:
- Maintenance: `Machine.ToMaintenanceRequestProto(operation, reference)`.
- Metadata-update: `Machine.ToMetadataUpdateRequestProto(labels)` (reads the current site-metadata Name from stored metadata, falling back to `m.ID`).
- Handler simplified to one-liners at both sites.

Tests added!

Signed-off-by: Chet Nichols III <chetn@nvidia.com>
@chet chet requested a review from a team as a code owner June 2, 2026 23:53
@chet
Copy link
Copy Markdown
Contributor Author

chet commented Jun 2, 2026

@coderabbitai Hey! There is a .coderabbit.yaml in rest-api/.coderabbit.yaml in this repo that you can use, with a rest-api/AGENTS.md with general guidelines. This PR actually should be good to go based on previous work we've done (we just brought all of this code over from https://github.com/NVIDIA/infra-controller-rest, where you might have a lot of context), but looping you in directly since we haven't gotten the plumbing wired up yet. I think we should be good here, since we've been working through things over every PR, but it's always good to have you sign-off/verify. Thank you!

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 2, 2026

🔐 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-02 23:55:28 UTC | Commit: be7b093

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 2, 2026

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details and diff
gh pr view 2139 --repo NVIDIA/infra-controller --json files,additions,deletions,title,body 2>/dev/null | jq '.files[].path'
echo "---"
gh pr diff 2139 --repo NVIDIA/infra-controller 2>/dev/null

Length of output: 6598


🏁 Script executed:

#!/bin/bash
# Read AGENTS.md and .coderabbit.yaml
cat rest-api/AGENTS.md 2>/dev/null || echo "AGENTS.md not found"
echo "==="
cat rest-api/.coderabbit.yaml 2>/dev/null || echo ".coderabbit.yaml not found"

Length of output: 30368


🏁 Script executed:

#!/bin/bash
# Read the Machine model and handler files
fd -e go . rest-api/ | xargs grep -l -i "machine" 2>/dev/null | head -30

Length of output: 1764



@chet — happy to give this a proper look! I've read through the diff, rest-api/AGENTS.md, and rest-api/.coderabbit.yaml, and cross-checked against the conventions we've been building up.

Verdict: LGTM ✅

Here's what I verified against the AGENTS.md proto-conversion rules:

Check Result
Both methods on *Machine entity (no API request body → rule 3)
ToMaintenanceRequestProto / ToMetadataUpdateRequestProto naming pattern
operation, reference, labels passed as side-input args (rule 4)
Nil-receiver guards on both methods
Handler reduced to one-liners — no inline proto assembly remaining
Tests cover enable+ref, disable+nil, metadata nil-fallback, and stored-name paths

One small improvement worth calling out: the new ToMetadataUpdateRequestProto adds a != "" guard on the Name check:

if m.Metadata != nil && m.Metadata.Metadata != nil && m.Metadata.Metadata.Name != "" {

The old handler didn't have this, so it would have forwarded an empty string as the Name if the stored metadata had one. The new code correctly falls back to m.ID in that case — a quiet correctness win on top of the refactor.

Everything looks production-grade here. Ship it! 🚀

(=^・ω・^=)

[approve]


🧠 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 the `infra-controller-rest` codebase (Go, Echo v4), proto conversion follows strict receiver-method conventions documented in `rest-api/AGENTS.md`:
1. Primary entity ↔ proto: `(*T).ToProto(...)` and `(*T).FromProto(p *protoT, ...)` on DB models in `db/pkg/db/model/`.
2. API request → proto: `(*APIXCreateRequest).ToProto(...)` and `(*APIXUpdateRequest).ToProto(...)` on API request types in `api/pkg/api/model/`.
3. Entity-only proto shapes (delete, maintenance): `(*T).ToDeletionRequestProto()`, `(*T).ToMaintenanceRequestProto(...)` on DB models.
`ToProto` does not return errors — validation lives in `(req *APIX).Validate() error` which must be called before `ToProto`. `Vpc` is the reference for rules 1–3; `InstanceType` is the reference for everything else (typed slice, typed map, ozzo composition).

Learnt from: CR
Repo: NVIDIA/infra-controller PR: 0
File: rest-api/AGENTS.md:0-0
Timestamp: 2026-06-02T02:05:39.699Z
Learning: Applies to rest-api/api/pkg/api/model/*.go : API request types should define `ToProto()` methods that convert request-specific fields to proto request types, not free functions

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.

Learnt from: CR
Repo: NVIDIA/infra-controller PR: 0
File: rest-api/AGENTS.md:0-0
Timestamp: 2026-06-02T02:05:39.699Z
Learning: Applies to rest-api/**/*.go : Named types that represent domain concepts with conversion needs should own their proto behavior via receiver methods; free functions like `LabelsToProto()` are anti-patterns

Learnt from: CR
Repo: NVIDIA/infra-controller PR: 0
File: rest-api/AGENTS.md:0-0
Timestamp: 2026-06-02T02:05:39.699Z
Learning: Applies to rest-api/**/*.go : `FromProto` methods must be pointer-receivers that mutate the receiver in place, return no error, treat `nil` proto as a no-op, preserve required ID fields, and clear optional pointer fields when proto omits them or values are invalid

Learnt from: CR
Repo: NVIDIA/infra-controller PR: 0
File: rest-api/AGENTS.md:0-0
Timestamp: 2026-06-02T02:05:39.699Z
Learning: Applies to rest-api/**/*.go : Proto conversion methods should be receiver methods on types, not free functions: `func (m *T) ToProto(...) *protoT` and `func (m *T) FromProto(p *protoT, ...)`

@chet chet changed the title chore: Apply our new FromProto/ToProto modeling to Machine chore(rest-api): Apply our new FromProto/ToProto modeling to Machine Jun 3, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 3, 2026

🔍 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant