HYPERFLEET-1103 - feat:restructure TypeSpec sources into core, gcp#49
HYPERFLEET-1103 - feat:restructure TypeSpec sources into core, gcp#49rh-amarin wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR creates a new core TypeSpec entrypoint at core/main.tsp (API namespace, server, route, Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rh-amarin
force-pushed
the
restructure-provider-dirs
branch
from
e7d8e7d to
7c09f0d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@gcp/models/nodepool/example_post.tsp`:
- Around line 1-2: The architecture docs still describe the old "alias-based
type switching" (aliases-core.tsp / aliases-gcp.tsp) but this PR replaces that
with provider-specific entrypoints and direct shared imports (see import
"./model.tsp" and import "../../../shared/models/nodepools/model.tsp"); update
the architecture documentation to describe the new module composition: document
provider-specific entrypoints, how per-provider modules (e.g., GCP) import
shared contracts directly, and note the decoupled versioning and independent
contract ownership model that replaces aliases-core.tsp and aliases-gcp.tsp.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 313724a6-8f41-45f0-8b36-25be0c88aa35
📒 Files selected for processing (36)
.github/workflows/ci.yml.github/workflows/release.ymlaliases-core.tspaliases-gcp.tspaliases.tspbuild-schema.shcore/main.tspcore/models/cluster/example_cluster.tspcore/models/cluster/example_patch.tspcore/models/cluster/example_post.tspcore/models/cluster/model.tspcore/models/nodepool/example_nodepool.tspcore/models/nodepool/example_patch.tspcore/models/nodepool/example_post.tspcore/models/nodepool/model.tspcore/services/statuses-internal.tspgcp/main.tspgcp/models/cluster/example_cluster.tspgcp/models/cluster/example_patch.tspgcp/models/cluster/example_post.tspgcp/models/cluster/model.tspgcp/models/nodepool/example_nodepool.tspgcp/models/nodepool/example_patch.tspgcp/models/nodepool/example_post.tspgcp/models/nodepool/model.tspmodels/compatibility/README.mdschemas/core/openapi.yamlschemas/core/swagger.yamlshared/models/clusters/model.tspshared/models/common/model.tspshared/models/nodepools/model.tspshared/models/statuses/example_adapter_status.tspshared/models/statuses/model.tspshared/services/clusters.tspshared/services/nodepools.tspshared/services/statuses.tsp
💤 Files with no reviewable changes (4)
- aliases-gcp.tsp
- models/compatibility/README.md
- aliases.tsp
- aliases-core.tsp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
schemas/core/openapi.yaml (1)
4-4:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRegenerate schemas to sync version and resolve pipeline failures.
The version field shows
1.0.14but pipeline expects1.0.15. The CI/release workflows extract the version fromcore/main.tspper layer 2 of this PR stack. Re-run./build-schema.sh core --swaggerto regenerate this file and resolve the out-of-sync state flagged by CI.🤖 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 `@schemas/core/openapi.yaml` at line 4, The OpenAPI schema version is out of sync (shows 1.0.14 vs expected 1.0.15); regenerate the schema so the version in the version field matches core/main.tsp by re-running the schema build command (run the build-script used in CI, e.g. ./build-schema.sh core --swagger) and commit the updated schemas so the version in schemas/core/openapi.yaml is updated to 1.0.15..github/workflows/ci.yml (1)
50-66:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCore-only version gating breaks provider version decoupling.
Line 50 checks only
core/main.tsp, so a GCP-only contract change still fails unless core is bumped. That re-couples provider versions and contradicts the decoupled-contract objective.🤖 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 @.github/workflows/ci.yml around lines 50 - 66, The version check currently reads core/main.tsp into CURRENT and fails the job if CURRENT isn't bumped; change the CI so it only enforces the core version bump when core/main.tsp was actually modified in the PR: determine changed files (e.g., via git diff --name-only against the target branch) and if core/main.tsp is not among them, skip the core version gating and exit 0; otherwise keep the existing logic using CURRENT, LATEST and HIGHEST to validate that core/main.tsp was bumped..github/workflows/release.yml (1)
31-50:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSingle core-derived tag blocks GCP-only releases.
Line 31-Line 38 derive a single tag from
core/main.tsp; Line 45-Line 50 then skip the release when that tag exists. That means provider-specific changes ingcpcan be merged without producing a new release artifact.🤖 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 @.github/workflows/release.yml around lines 31 - 50, The workflow currently builds a single TAG from VERSION (extracted from core/main.tsp) and the check_tag step uses that to skip releases, which prevents provider-specific (e.g., GCP) changes from producing releases; update the tag-generation logic so the TAG is unique per release target (for example append the provider name or other target identifier or commit SHA to VERSION), then update the Check if release already exists step (id: check_tag, env: TAG) to use that per-target TAG so only exact-matching provider tags are considered existing; locate where VERSION is set (core/main.tsp extraction) and where TAG is exported and change TAG composition consistently across the workflow.
🤖 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 `@build-schema.sh`:
- Around line 62-63: The script uses raw $PROVIDER to build paths (e.g.,
PROVIDER_ENTRY="${PROVIDER}/main.tsp") which allows directory traversal or
slashes; add an allowlist validation for PROVIDER before any use (e.g., validate
that PROVIDER matches a safe regex such as only alphanumerics, hyphen and
underscore, and no slashes or dots) and exit with an error if it fails; apply
this validation once near the top of the script and use the validated variable
for all subsequent uses (including where PROVIDER is used to construct compile
source and output directories) so functions/variables like PROVIDER_ENTRY and
any compile/output path constructions never see unvalidated input.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 50-66: The version check currently reads core/main.tsp into
CURRENT and fails the job if CURRENT isn't bumped; change the CI so it only
enforces the core version bump when core/main.tsp was actually modified in the
PR: determine changed files (e.g., via git diff --name-only against the target
branch) and if core/main.tsp is not among them, skip the core version gating and
exit 0; otherwise keep the existing logic using CURRENT, LATEST and HIGHEST to
validate that core/main.tsp was bumped.
In @.github/workflows/release.yml:
- Around line 31-50: The workflow currently builds a single TAG from VERSION
(extracted from core/main.tsp) and the check_tag step uses that to skip
releases, which prevents provider-specific (e.g., GCP) changes from producing
releases; update the tag-generation logic so the TAG is unique per release
target (for example append the provider name or other target identifier or
commit SHA to VERSION), then update the Check if release already exists step
(id: check_tag, env: TAG) to use that per-target TAG so only exact-matching
provider tags are considered existing; locate where VERSION is set
(core/main.tsp extraction) and where TAG is exported and change TAG composition
consistently across the workflow.
In `@schemas/core/openapi.yaml`:
- Line 4: The OpenAPI schema version is out of sync (shows 1.0.14 vs expected
1.0.15); regenerate the schema so the version in the version field matches
core/main.tsp by re-running the schema build command (run the build-script used
in CI, e.g. ./build-schema.sh core --swagger) and commit the updated schemas so
the version in schemas/core/openapi.yaml is updated to 1.0.15.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 113d6839-9474-42ff-a3d4-e90a36a820e6
📒 Files selected for processing (36)
.github/workflows/ci.yml.github/workflows/release.ymlaliases-core.tspaliases-gcp.tspaliases.tspbuild-schema.shcore/main.tspcore/models/cluster/example_cluster.tspcore/models/cluster/example_patch.tspcore/models/cluster/example_post.tspcore/models/cluster/model.tspcore/models/nodepool/example_nodepool.tspcore/models/nodepool/example_patch.tspcore/models/nodepool/example_post.tspcore/models/nodepool/model.tspcore/services/statuses-internal.tspgcp/main.tspgcp/models/cluster/example_cluster.tspgcp/models/cluster/example_patch.tspgcp/models/cluster/example_post.tspgcp/models/cluster/model.tspgcp/models/nodepool/example_nodepool.tspgcp/models/nodepool/example_patch.tspgcp/models/nodepool/example_post.tspgcp/models/nodepool/model.tspmodels/compatibility/README.mdschemas/core/openapi.yamlschemas/core/swagger.yamlshared/models/clusters/model.tspshared/models/common/model.tspshared/models/nodepools/model.tspshared/models/statuses/example_adapter_status.tspshared/models/statuses/model.tspshared/services/clusters.tspshared/services/nodepools.tspshared/services/statuses.tsp
💤 Files with no reviewable changes (4)
- models/compatibility/README.md
- aliases-gcp.tsp
- aliases.tsp
- aliases-core.tsp
| PROVIDER_ENTRY="${PROVIDER}/main.tsp" | ||
| if [ ! -f "$PROVIDER_ENTRY" ]; then |
There was a problem hiding this comment.
Validate provider names before using them in filesystem paths.
Line 62 and Line 100 trust raw $PROVIDER in path construction. Inputs containing / or .. can escape intended locations (compile source and output dir). Add an allowlist check before using $PROVIDER.
Suggested fix
PROVIDER="$1"
shift
+# Restrict provider to simple directory names (no path separators/traversal)
+if [[ ! "$PROVIDER" =~ ^[a-zA-Z0-9_-]+$ ]]; then
+ echo -e "${RED}Error: Invalid provider name: ${PROVIDER}${NC}"
+ echo "Provider must contain only letters, numbers, '-' or '_'"
+ exit 1
+fi
+
# Check if provider looks like an option (starts with -)
if [[ "$PROVIDER" == -* ]]; thenAlso applies to: 99-101
🤖 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 `@build-schema.sh` around lines 62 - 63, The script uses raw $PROVIDER to build
paths (e.g., PROVIDER_ENTRY="${PROVIDER}/main.tsp") which allows directory
traversal or slashes; add an allowlist validation for PROVIDER before any use
(e.g., validate that PROVIDER matches a safe regex such as only alphanumerics,
hyphen and underscore, and no slashes or dots) and exit with an error if it
fails; apply this validation once near the top of the script and use the
validated variable for all subsequent uses (including where PROVIDER is used to
construct compile source and output directories) so functions/variables like
PROVIDER_ENTRY and any compile/output path constructions never see unvalidated
input.
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
1 participant
Summary
GCP team should have ownership for their specific GCP contract in the hyperfleet-api-spec repository
The PR also decouples the versioning of the two contracts, keeping a separate main.tsp file for each contract version