Skip to content

fix(rest): enable Flow for new sites by default - #5629

Merged
kunzhao-nv merged 2 commits into
NVIDIA:mainfrom
kunzhao-nv:rest-flow-default-enabled
Sep 1, 2026
Merged

fix(rest): enable Flow for new sites by default#5629
kunzhao-nv merged 2 commits into
NVIDIA:mainfrom
kunzhao-nv:rest-flow-default-enabled

Conversation

@kunzhao-nv

@kunzhao-nv kunzhao-nv commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

REST-created Sites already enable the default networking posture, but Flow remains disabled and requires a follow-up Site update before Flow-backed operations are available. This change enables Flow as part of the new-Site default posture and verifies that the API response and persisted Site configuration agree.

Existing Site records are unchanged. The new default applies only when REST creates a Site. #5635 tracks reconciliation of this value from Site Agent inventory.

Related issues

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

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

Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
@kunzhao-nv
kunzhao-nv requested a review from a team as a code owner September 1, 2026 17:33
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

🔐 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-09-01 17:37:05 UTC | Commit: 2e3123a

@thossain-nv thossain-nv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kunzhao-nv This is ok as a stop-gap but can we have a more informed way of enabling this flag? Can Site Agent discover Flow connection on startup and report it in the Site config inventory?

@thossain-nv thossain-nv added the rest-api Add this label when an issue or PR concerns NICo REST API label Sep 1, 2026 — with ChatGPT Codex Connector
@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: eaeb3bb9-d6cd-4241-aafb-fa1ed26cd2d5

📥 Commits

Reviewing files that changed from the base of the PR and between 2e3123a and 91def8b.

📒 Files selected for processing (2)
  • rest-api/docs/index.html
  • rest-api/openapi/spec.yaml

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.


Summary by CodeRabbit

  • New Features

    • Newly created Sites now enable Flow capabilities by default, alongside native networking and network security features.
    • API documentation reflects the default capability settings for new Sites.
  • Bug Fixes

    • Site creation responses and saved configurations now consistently include the Flow capability.

Walkthrough

Newly created Sites enable NativeNetworking, NetworkSecurityGroup, and Flow by default. Handler tests verify the API response and persisted configuration. The OpenAPI specification documents these defaults and updates its example.

Changes

Site capability defaults

Layer / File(s) Summary
Document default capabilities
rest-api/openapi/spec.yaml
The OpenAPI specification documents the default-enabled capabilities and updates the example values.
Apply and validate capability defaults
rest-api/api/pkg/api/handler/site.go, rest-api/api/pkg/api/handler/site_test.go
Site creation enables Flow. Tests verify the API response and persisted configuration.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 91def

This localized change enables Flow by default for newly REST-created Sites while leaving existing Sites unchanged; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly and concisely describes the main change: enabling Flow by default for new REST-created Sites.
Description check ✅ Passed The description directly explains the Flow default change, its scope, verification, and related issue.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@kunzhao-nv

Copy link
Copy Markdown
Contributor Author

@kunzhao-nv This is ok as a stop-gap but can we have a more informed way of enabling this flag? Can Site Agent discover Flow connection on startup and report it in the Site config inventory?

That makes sense. Site Agent already verifies the Flow connection through the Flow Version RPC during startup, so it can report Flow availability through Site config inventory and make that the source of truth. I’ll keep this PR as the stop-gap default and follow up with inventory-based discovery.

…bled

Signed-off-by: Kun Zhao <kunzhao@nvidia.com>

# Conflicts:
#	rest-api/docs/index.html
@kunzhao-nv
kunzhao-nv enabled auto-merge (squash) September 1, 2026 19:20
@kunzhao-nv
kunzhao-nv merged commit 0b77d27 into NVIDIA:main Sep 1, 2026
125 checks passed
kunzhao-nv added a commit that referenced this pull request Sep 2, 2026
Site configuration currently retains the default Flow capability without
reconciling the Site Agent configuration that owns the integration.

This change:

- adds `flow_enabled` to `SiteAgentBuildInfo` and reports the Site
Agent's configured `flowGrpcEnabled` value;
- reconciles the reported value in both directions into
`site.config.flow` without replacing unrelated Site configuration;
- removes `flow` from the writable Site capabilities contract and
explicitly rejects attempts to update it through REST; and
- keeps Flow connection health separate from the configured capability
so existing metrics remain the health signal.

## Related issues

- Closes #5635
- Follow-up to #5629

## Type of Change

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

## Breaking Changes

- [x] **This PR contains breaking changes**

`capabilities.flow` is no longer accepted by the Site update API because
Site Agent inventory is the authoritative writer.

## Testing

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

## Additional Notes

---------

Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rest-api Add this label when an issue or PR concerns NICo REST API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants