SDL parity tests #169
SDL parity tests #169
Conversation
WalkthroughAdds SDL JSON/YAML schemas, a Go SchemaValidator, extensive schema and parity tests (Go + TypeScript), fixture generation tooling, updated manifest shapes (resource units as strings, renamed signed_by/all_of/any_of), deterministic ordering in group builders, and CI/make targets to run parity checks. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / CI
participant FS as Filesystem (testdata)
participant Gen as generate-sdl-fixtures (Go tool)
participant GoSDL as go/sdl.Read + SchemaValidator
participant TS as TypeScript parity tests
participant Reporter as Test runner / CI job
Dev->>FS: push repository with input.yaml fixtures
Reporter->>Gen: run `generate-sdl-fixtures`
Gen->>FS: read `input.yaml`
Gen->>GoSDL: call sdl.ReadFile(input.yaml)
GoSDL->>GoSDL: validateInputAgainstSchema (embedded schema)
GoSDL-->>Gen: return SDL, manifest & groups
Gen->>FS: write manifest.json + groups.json
Reporter->>TS: run TypeScript parity tests (uses fixtures)
TS->>FS: read input.yaml, manifest.json, groups.json
TS->>GoSDL: (via parity logic) parse/produce outputs or compare
TS-->>Reporter: test results
Reporter->>Dev: CI status (success/failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
vertex451
force-pushed
the
artem/sdl-parity-tests
branch
from
c464548 to
c669081
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
go/sdl/go.mod (1)
7-15: gojsonschema dependency is used in production JWT schema validationThe
gojsonschemadependency is imported ingo/util/jwt/schema.gofor production JWT schema validation (initialized at package level), in addition to its use in SDL parity tests. The dependency is appropriate for these use cases, though be aware it initializes at package load time and uses reflection, which incurs startup overhead.ts/src/sdl/SDL/SDL.ts (1)
834-846: Essential change: Deterministic storage ordering for parity testing.Sorting storage keys alphabetically before mapping ensures consistent, reproducible manifest output. This is critical for cross-language parity validation.
♻️ Duplicate comments (1)
go/sdl/groupBuilder_v2.go (1)
100-121: Sorted iteration over storage volumes for deterministic manifestsUsing
storageNames+sort.Stringsbefore appendingStorageParamseliminates Go map iteration nondeterminism and keeps v2 output stable across runs and in parity tests. Implementation mirrors the v2.1 path and looks correct.
🧹 Nitpick comments (11)
go/sdl/groupBuilder_v2_1.go (1)
100-131: Deterministic storage params ordering is correctSorting
svc.Params.Storagekeys before buildingparams.Storageremoves map‑iteration nondeterminism and aligns with parity/determinism needs. Logic is sound and nil‑safe; any future refactor could share this helper with the v2 builder, but it’s not required now.make/test.mk (1)
51-63: Parity test workflow is solid; align TS path with existingTS_ROOTThe new
generate-sdl-fixturesandtest-sdl-paritytargets wire the Go and TS parity tests together cleanly, and the sequencing (fixtures → Go tests → TS tests) makes sense.For consistency with
test-ts/test-coverage-ts, consider using$(TS_ROOT)instead of a hard-codedtspath:- @cd ts && npm test -- --testPathPattern=parity.spec.ts + @cd $(TS_ROOT) && npm test -- --testPathPattern=parity.spec.tsThis avoids future drift if the TS root directory is ever renamed.
go/sdl/cmd/generate-sdl-fixtures/main.go (1)
13-13: Consider making the fixtures root path configurable.The hard-coded relative path
"../../../testdata/sdl"assumes the command is executed from a specific directory (likelygo/sdl/cmd/generate-sdl-fixtures). If the tool is run from a different location or installed elsewhere, it will fail.Consider accepting the fixtures root as a command-line argument with a default value:
+import ( + "flag" + // ... existing imports +) + func main() { - fixturesRoot := "../../../testdata/sdl" + fixturesRoot := flag.String("fixtures", "../../../testdata/sdl", "Path to SDL fixtures root directory") + flag.Parse() + versions := []string{"v2.0", "v2.1"} for _, version := range versions { - versionDir := filepath.Join(fixturesRoot, version) + versionDir := filepath.Join(*fixturesRoot, version)testdata/sdl/v2.0/persistent-storage/groups.json (1)
1-87: Add missing trailing newline.The file is missing a trailing newline at EOF. While some build systems are forgiving, most linters and Git conventions expect JSON files to end with a newline character for POSIX compliance.
Add a newline after the closing bracket at line 87.
] +testdata/sdl/v2.0/simple/groups.json (1)
1-67: Add missing trailing newline.Per the file structure, the JSON is valid but ends without a trailing newline. Add a newline after the closing bracket for POSIX compliance.
] +testdata/sdl/v2.0/storage-classes/groups.json (1)
1-87: Add missing trailing newline.The file is missing a trailing newline at EOF, violating POSIX conventions and typical linter expectations.
] +specs/sdl/sdl-input.schema.yaml (1)
10-24: Consider usingintegertype for counters instead ofnumberFields like
deployment.*.*.count(and similar count-like fields elsewhere) are conceptually integers. Using"type": "integer"instead of"type": "number"would prevent fractional values from passing schema validation while keeping current fixtures valid.Also applies to: 41-65
specs/sdl/groups.schema.yaml (1)
45-65: Tighten groups schema forcountandprice.denomTo better reflect the data model and avoid subtle mistakes:
resources[*].countshould be an integer (non-negative) rather than a genericnumber.pricecurrently requiresamountbut notdenom; in practice both are always present, so makingdenomrequired too would strengthen validation without affecting existing fixtures.go/sdl/parity_test.go (1)
17-46: Optional: reuse compiled JSON Schemas across fixtures
validateAgainstSchemareads and compiles the schema file for every manifest/groups validation. With many fixtures this adds unnecessary overhead. Consider caching compiled schemas keyed byschemaPath(e.g., package-levelmap[string]*gojsonschema.Schemaprotected bysync.Mutex/sync.Once) so each schema file is loaded and compiled only once per test run.Also applies to: 94-118
ts/src/sdl/SDL/parity.spec.ts (1)
81-101: Optional: cache compiled schemas to avoid recompiling on every fixture
validateAgainstSchemareads and compiles the YAML schema on every call. For many fixtures this adds overhead in test runs. You could cache the compiled Ajv validators byschemaPath(e.g., a simpleMap<string, ValidateFunction>) so each schema file is parsed/compiled only once per test run. This follows Ajv's recommended pattern of compiling once and reusing the validate function rather than recompiling on each invocation.specs/sdl/manifest.schema.yaml (1)
1-3: Add $schema and $id metadata for schema documentation and tooling.This is a new public schema file. It lacks standard JSON Schema metadata fields that aid documentation, versioning, and integration with schema validators.
Add
$schema(to declare the JSON Schema version) and$id(for schema identification and URL resolution) at the root level:+$schema: 'https://json-schema.org/draft/2020-12/schema' +$id: 'https://akash.network/schemas/sdl/manifest.schema.json' +title: Akash SDL Manifest Schema +description: Validates the structure of a compiled SDL manifest, including services, resources, and deployment configurations. items: additionalProperties: false properties:Also applies to: 328-328
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go/sdl/go.sumis excluded by!**/*.sumts/package-lock.jsonis excluded by!**/package-lock.jsonts/src/sdl/SDL/__snapshots__/SDL.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (50)
.github/workflows/tests.yaml(1 hunks).gitignore(1 hunks)go/sdl/cmd/generate-sdl-fixtures/main.go(1 hunks)go/sdl/go.mod(2 hunks)go/sdl/groupBuilder_v2.go(1 hunks)go/sdl/groupBuilder_v2_1.go(1 hunks)go/sdl/parity_test.go(1 hunks)make/test.mk(1 hunks)specs/sdl/groups.schema.yaml(1 hunks)specs/sdl/manifest.schema.yaml(1 hunks)specs/sdl/sdl-input.schema.yaml(1 hunks)specs/sdl/validation-limitations.md(1 hunks)testdata/sdl/invalid/credentials-missing-host.yaml(1 hunks)testdata/sdl/invalid/endpoint-not-used.yaml(1 hunks)testdata/sdl/invalid/invalid-port.yaml(1 hunks)testdata/sdl/invalid/missing-deployment.yaml(1 hunks)testdata/sdl/invalid/missing-image.yaml(1 hunks)testdata/sdl/invalid/missing-version.yaml(1 hunks)testdata/sdl/invalid/negative-cpu.yaml(1 hunks)testdata/sdl/invalid/persistent-without-mount.yaml(1 hunks)testdata/sdl/v2.0/gpu-basic/groups.json(1 hunks)testdata/sdl/v2.0/gpu-basic/input.yaml(1 hunks)testdata/sdl/v2.0/gpu-basic/manifest.json(1 hunks)testdata/sdl/v2.0/http-options/groups.json(1 hunks)testdata/sdl/v2.0/http-options/input.yaml(1 hunks)testdata/sdl/v2.0/http-options/manifest.json(1 hunks)testdata/sdl/v2.0/ip-endpoint/groups.json(1 hunks)testdata/sdl/v2.0/ip-endpoint/input.yaml(1 hunks)testdata/sdl/v2.0/ip-endpoint/manifest.json(1 hunks)testdata/sdl/v2.0/multiple-services/groups.json(1 hunks)testdata/sdl/v2.0/multiple-services/input.yaml(1 hunks)testdata/sdl/v2.0/multiple-services/manifest.json(1 hunks)testdata/sdl/v2.0/persistent-storage/groups.json(1 hunks)testdata/sdl/v2.0/persistent-storage/input.yaml(1 hunks)testdata/sdl/v2.0/persistent-storage/manifest.json(1 hunks)testdata/sdl/v2.0/simple/groups.json(1 hunks)testdata/sdl/v2.0/simple/input.yaml(1 hunks)testdata/sdl/v2.0/simple/manifest.json(1 hunks)testdata/sdl/v2.0/storage-classes/groups.json(1 hunks)testdata/sdl/v2.0/storage-classes/input.yaml(1 hunks)testdata/sdl/v2.0/storage-classes/manifest.json(1 hunks)testdata/sdl/v2.1/credentials/groups.json(1 hunks)testdata/sdl/v2.1/credentials/input.yaml(1 hunks)testdata/sdl/v2.1/credentials/manifest.json(1 hunks)testdata/sdl/v2.1/ip-endpoint/groups.json(1 hunks)testdata/sdl/v2.1/ip-endpoint/input.yaml(1 hunks)testdata/sdl/v2.1/ip-endpoint/manifest.json(1 hunks)ts/package.json(1 hunks)ts/src/sdl/SDL/SDL.ts(9 hunks)ts/src/sdl/SDL/parity.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
ts/src/sdl/SDL/parity.spec.ts (1)
ts/src/sdl/SDL/SDL.ts (3)
validate(128-138)validate(246-261)SDL(88-1315)
go/sdl/cmd/generate-sdl-fixtures/main.go (1)
go/sdl/sdl.go (1)
ReadFile(83-89)
go/sdl/parity_test.go (2)
go/sdl/sdl.go (1)
ReadFile(83-89)go/testutil/deployment.go (1)
DeploymentGroups(54-62)
ts/src/sdl/SDL/SDL.ts (2)
ts/src/sdl/index.ts (1)
SdlValidationError(28-28)ts/src/sdl/types.ts (1)
v3Sdl(100-105)
🪛 GitHub Actions: lint
go/sdl/cmd/generate-sdl-fixtures/main.go
[error] 87-87: gofmt formatting issue detected. File is not properly formatted. Run 'gofmt -w' or 'go fmt ./...' to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: coverage
🔇 Additional comments (42)
.gitignore (1)
27-34: Well-organized additions supporting the testing infrastructure.The new entries appropriately capture test/build artifacts and coverage outputs introduced by the SDL parity testing changes. The naming conventions align with existing patterns, and the logical grouping with comments enhances clarity.
testdata/sdl/invalid/missing-image.yaml (1)
1-30: Invalid SDL fixture for missing image looks correctSDL structure is consistent with other v2.0 fixtures, and omitting
services.web.imageshould exercise the intended invalid case.testdata/sdl/invalid/invalid-port.yaml (1)
1-32: Invalid‑port SDL fixture is well‑formed for the negative testThe SDL is structurally valid but uses
port: 99999, which is out of the allowed port range, making it a good fixture to assert that validation rejects out‑of‑range ports.testdata/sdl/v2.0/ip-endpoint/manifest.json (1)
1-103: ip‑endpoint manifest fixture appears structurally consistentThe manifest structure (group → service → resources/endpoints → expose with
ipandendpointSequenceNumber) looks consistent with the v2.0/ip‑endpoint scenario and suitable for parity tests. Assuming this JSON was generated by the fixture tool, it should accurately represent the expected manifest; if it was edited manually, it’s worth re‑generating once to ensure it matches generator output and the companiongroups.json.testdata/sdl/invalid/negative-cpu.yaml (1)
1-32: Negative‑CPU SDL fixture correctly targets invalid resource valuesUsing
cpu.units: -100min an otherwise valid deployment is an appropriate way to exercise validation for disallowing negative CPU values.ts/package.json (1)
69-81: AJV is correctly placed as a devDependencyAJV (
^8.12.0) is used only in test files (ts/src/sdl/SDL/parity.spec.ts), confirming it should remain indevDependenciesand not be added todependencies. No runtime code imports or depends on AJV.testdata/sdl/invalid/credentials-missing-host.yaml (1)
1-35: LGTM! Invalid fixture correctly tests missing host credential validation.This fixture appropriately tests the credential validation pathway by intentionally omitting the required
hostfield while providingusernameandpassword. The validation logic ints/src/sdl/SDL/SDL.ts(lines 301-310) will correctly reject this SDL.testdata/sdl/v2.0/http-options/manifest.json (1)
1-73: LGTM! HTTP options manifest fixture is well-structured.The manifest correctly defines HTTP options with appropriate retry policies and timeout configurations for parity testing.
testdata/sdl/v2.0/storage-classes/manifest.json (1)
1-117: LGTM! Storage classes manifest correctly demonstrates multiple storage volumes.The fixture properly defines storage with varying attributes (persistent flags, storage classes) and the storage params section follows the alphabetically sorted order (cache, data, logs) as enforced by the updated SDL.ts logic.
testdata/sdl/invalid/endpoint-not-used.yaml (1)
1-35: LGTM! Invalid fixture correctly tests unused endpoint detection.This fixture appropriately validates that endpoints defined but never referenced in service exposures are detected as errors. The
validateEndpointsUtilitymethod in SDL.ts will correctly flag this scenario.testdata/sdl/v2.0/gpu-basic/manifest.json (1)
1-76: LGTM! GPU manifest fixture correctly represents GPU configuration.The fixture properly defines GPU resources with NVIDIA RTX 3080 specification, suitable for testing GPU-enabled deployment parity.
ts/src/sdl/SDL/SDL.ts (5)
2-2: LGTM! Named import improves clarity.Using the named
loadimport fromjs-yamlis more explicit and follows better practices than relying on default exports.
114-121: LGTM! Version field validation enhances robustness.Adding an explicit check for the required
versionfield with a clear error message improves SDL validation. The type cast tov3Sdlappropriately follows the validation.
285-299: LGTM! Service image and port validations strengthen SDL integrity.Both validation methods correctly enforce essential constraints:
- Empty image names are rejected
- Port values are constrained to valid range (1-65535)
The error messages are clear and helpful.
848-864: LGTM! Proper null handling and deterministic storage ordering.The method correctly handles
undefined/nullparams by returningnull, and ensures deterministic storage parameter ordering through alphabetical sorting.
899-916: LGTM! Conditional params assignment prevents unnecessary fields.Computing params via
v3ManifestServiceParamsand only assigning when not null prevents adding empty params fields to the manifest, resulting in cleaner output..github/workflows/tests.yaml (1)
57-76: LGTM! SDL parity CI job is properly configured.The new job correctly mirrors the setup steps from the coverage job and executes the SDL parity tests. The setup includes both Node.js and Go environments as required for cross-language parity validation.
testdata/sdl/v2.1/credentials/manifest.json (1)
1-75: LGTM! Credentials manifest correctly includes all required fields.This fixture appropriately tests the valid credentials scenario with all required fields (host, email, username, password) present, complementing the invalid credentials fixtures.
testdata/sdl/v2.0/gpu-basic/groups.json (1)
1-65: GPU basic group fixture looks structurally consistentThe JSON structure (requirements/resources/price) is consistent with other group fixtures, and the numeric values (CPU/memory/storage/GPU/price) look coherent for a minimal GPU case.
testdata/sdl/v2.0/multiple-services/groups.json (1)
1-130: Multiple-services group fixture is coherent and well-formedRequirements, resource variants, and pricing are internally consistent and align with the structure used in the other group fixtures in this PR.
testdata/sdl/invalid/missing-deployment.yaml (1)
1-27: Invalid “missing deployment” fixture matches its intentYAML is structurally valid, and omitting the
deploymentblock accurately represents the intended invalid case for tests.testdata/sdl/invalid/persistent-without-mount.yaml (1)
1-35: Persistent-without-mount invalid fixture is clearly definedThe compute/storage and deployment sections are well-formed, and the missing mount for a
persistent: truevolume cleanly expresses the invalid scenario the tests will exercise.testdata/sdl/v2.0/http-options/input.yaml (1)
1-50: HTTP-options SDL fixture is well-structured and expressiveThe
http_optionsblock and its parameters (timeouts, retries, next_cases) are clearly defined, and the compute/placement/deployment sections follow the established SDL v2.0 pattern, making this a good parity fixture.testdata/sdl/v2.0/ip-endpoint/input.yaml (1)
1-49: IP-endpoint SDL fixture cleanly exercises named IP exposureThe service/expose blocks correctly reference
ip: myendpoint, and the matchingendpoints.myendpointdefinition (kindip) plus standard compute/placement/deployment make this a solid parity case.testdata/sdl/v2.1/credentials/groups.json (1)
1-59: Credentials group fixture matches existing group schema usageThe requirements, resource definition, and pricing fields align with the other v2.0/v2.1 group fixtures and look appropriate for a credentials-focused parity test.
testdata/sdl/v2.0/simple/input.yaml (1)
1-47: LGTM! Well-structured SDL v2.0 test fixture.The fixture provides good coverage with multiple expose configurations (HTTP and UDP), placement attributes, and signedBy constraints with both anyOf and allOf.
testdata/sdl/v2.0/persistent-storage/manifest.json (1)
1-92: LGTM! Comprehensive manifest fixture for persistent storage.The manifest correctly captures persistent storage configuration with volume attributes (persistent: true, class: beta2) and mount parameters. The structure aligns with the expected manifest schema for v2.0.
testdata/sdl/invalid/missing-version.yaml (1)
1-31: LGTM! Valid negative test case for missing version.This fixture appropriately tests SDL validation by omitting the required
versionfield while maintaining otherwise valid structure. This ensures error handling for missing version information is properly exercised.testdata/sdl/v2.0/gpu-basic/input.yaml (1)
22-27: LGTM! Well-structured GPU resource configuration.The GPU attributes correctly specify the vendor (nvidia) and model (rtx3080) in nested format, which aligns with SDL v2.0 GPU specifications. This provides good test coverage for GPU-enabled deployments.
testdata/sdl/v2.0/http-options/groups.json (1)
1-59: LGTM! Valid groups fixture with proper structure.The groups data correctly defines requirements (signed_by, attributes), resources (compute specs, endpoints), and pricing. The 18-decimal precision for the amount field is standard for cryptocurrency denominations.
testdata/sdl/v2.1/ip-endpoint/input.yaml (1)
67-72: LGTM! Proper v2.1 IP endpoint configuration.The endpoints section correctly defines IP endpoints (endpoint1, endpoint2) which are referenced by the services' expose configurations. This provides good test coverage for the v2.1 IP endpoint feature.
testdata/sdl/v2.0/persistent-storage/input.yaml (1)
13-17: LGTM! Correct persistent storage configuration.The fixture properly links storage parameters (lines 14-17) with the compute profile's storage definition (lines 27-31). The storage volume is correctly marked as persistent with a storage class, and the mount point is properly specified.
Also applies to: 27-31
go/sdl/cmd/generate-sdl-fixtures/main.go (1)
60-60: File permissions0644are appropriate for generated test fixtures.The codebase shows a consistent pattern: test and configuration files use
0644permissions (owner read/write, group/others read-only), while sensitive files like key imports and governance data use0o600. Since the manifest and groups JSON files are test fixtures intended to be committed and read by the build system,0644is the correct choice.specs/sdl/validation-limitations.md (1)
1-12: All file references in the documentation are accurate. The schema filespecs/sdl/sdl-input.schema.yamland parser filesgo/sdl/v2.goandgo/sdl/v2_1.goall exist at the paths specified in the documentation.testdata/sdl/v2.1/ip-endpoint/manifest.json (1)
36-45: Verify if mixed endpoint structures between services are intentional.The manifest defines asymmetric endpoint configurations: the "api" service (lines 36-45) has consistent endpoints with both
kindandsequence_number, while the "web" service (lines 104-120) has mixed endpoints—some with onlysequence_number, others with both fields. Confirm whether this variation represents intentional test coverage for different endpoint types or if it's an inconsistency requiring alignment.Also applies to: 104-120
testdata/sdl/v2.1/credentials/input.yaml (1)
1-43: Schema reference and fixture consistency verified.The relative schema path
../../../../specs/sdl/sdl-input.schema.yamlis correct and the schema file exists. The input.yaml file is consistent with its corresponding groups.json and manifest.json fixtures: the service "private-app" maps to the "akash" group, and all resource specifications (CPU 500m, memory 1Gi, storage 5Gi, pricing 250 uakt) and credentials match across all three files.testdata/sdl/v2.1/ip-endpoint/groups.json (1)
1-114: ip-endpoint groups fixture shape looks consistent with schemaThe group structure (requirements.signed_by/attributes, resources with cpu/memory/storage/gpu/endpoints, count, and price) matches the new
groups.schema.yamlexpectations and provides good coverage of endpoint kinds/sequence numbers. I don’t see structural or obvious value issues here.testdata/sdl/v2.0/storage-classes/input.yaml (1)
1-59: Storage-classes SDL input aligns with the new SDL input schemaService, profiles (compute/placement), and deployment sections — including per-volume storage attributes and the
signedBy/pricing structure — all line up withsdl-input.schema.yaml. This fixture should be a solid basis for exercising storage-class handling.testdata/sdl/v2.0/multiple-services/manifest.json (1)
1-193: multiple-services manifest fixture is internally consistentService definitions (images, env, counts), inter-service expose wiring, resource IDs, and endpoint/
httpOptionsstructures are coherent and match what the v2.0 multiple-services SDL input describes. This looks like a good “golden” manifest for parity tests.testdata/sdl/v2.0/multiple-services/input.yaml (1)
1-93: multiple-services SDL input matches schema and manifest/groups fixturesThe service graph (web → api → db), compute/placement profiles, pricing, and deployment counts are coherent and line up with the generated manifest/groups fixtures. The
$schemaheader also correctly referencessdl-input.schema.yaml.specs/sdl/manifest.schema.yaml (2)
140-157: Clarify intentional structural difference between params.storage and resources.storage.Storage appears in two locations with differing structures:
- params.storage (lines 140-157): Array of objects with
{name, mount, readOnly}- resources.storage (lines 277-313): Array of objects with
{name, size, attributes}These serve different purposes, but the schema does not document this. Confirm this is intentional and document the distinction.
Verify that these storage structures represent different semantic concepts (e.g., params storage = mount points, resources storage = resource specifications) and not a schema inconsistency.
Also applies to: 277-313
67-92: httpOptions is universally required and consistently applied across all protocols in valid manifests.Verification of test fixtures confirms that all nine manifest files (v2.0 and v2.1) include httpOptions for every expose entry, regardless of protocol type (TCP, UDP, HTTP). No counter-examples exist where expose entries lack httpOptions. Non-HTTP protocols like TCP and UDP are fully supported and already accompanied by httpOptions in production fixtures, indicating this is an intentional schema design rather than an oversight. The concern that non-HTTP services would be rejected is unfounded—they are validated and expected to include httpOptions as part of the uniform expose specification.
vertex451
force-pushed
the
artem/sdl-parity-tests
branch
2 times, most recently
from
37393d8 to
204e70b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ts/src/sdl/SDL/parity.spec.ts (1)
163-169: Bug:finalValueis computed but immediately overwritten, making the clamping logic a no-op.This was flagged in a previous review. Line 167 assigns
finalValue(with the clampedendpointSequenceNumber), but line 169 immediately overwrites it with the originalvalue, negating allfinalValueadjustments.if (key === "endpointSequenceNumber" && typeof value === "number" && value > 1) { finalValue = 1; } normalized[normalizedKey] = finalValue; - - normalized[normalizedKey] = value;
🧹 Nitpick comments (4)
specs/sdl/sdl-input.schema.yaml (2)
211-215: Fix indentation inconsistency foraccept.items.The
itemsandtypeunderaccepthave extra indentation compared to sibling properties.accept: - items: - type: string - type: array + items: + type: string + type: array
287-291: Consider addingservicesandprofilesto required fields.Currently only
versionanddeploymentare required. While semantic validation is deferred to runtime, a valid SDL structurally requiresservicesandprofilessections. Adding them torequiredwould catch malformed SDLs earlier during IDE validation.required: - version - deployment + - services + - profilests/src/sdl/SDL/SDL.ts (1)
290-299: LGTM with minor suggestion.Port validation logic is correct. Consider a clearer error message:
- `Service "${serviceName}" has invalid port value. Port must be 0 < value <= 65535.`, + `Service "${serviceName}" has invalid port value. Port must be between 1 and 65535.`,ts/src/sdl/SDL/parity.spec.ts (1)
404-413: Consider testing invalid fixtures against both beta2 and beta3.Invalid fixtures are only tested with
beta3. If validation differs between versions (e.g., the invalid-port.yaml uses version "2.0"), testing only beta3 may miss version-specific edge cases.entries.forEach((filename) => { ["beta2", "beta3"].forEach((version) => { it(`should reject ${filename} (${version})`, () => { const input = fs.readFileSync( path.join(invalidDir, filename), "utf8", ); expect(() => SDL.fromString(input, version as "beta2" | "beta3")).toThrow(); }); }); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go/sdl/go.sumis excluded by!**/*.sumts/package-lock.jsonis excluded by!**/package-lock.jsonts/src/sdl/SDL/__snapshots__/SDL.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (50)
.github/workflows/tests.yaml(1 hunks).gitignore(1 hunks)go/sdl/cmd/generate-sdl-fixtures/main.go(1 hunks)go/sdl/go.mod(2 hunks)go/sdl/groupBuilder_v2.go(1 hunks)go/sdl/groupBuilder_v2_1.go(1 hunks)go/sdl/parity_test.go(1 hunks)make/test.mk(1 hunks)specs/sdl/groups.schema.yaml(1 hunks)specs/sdl/manifest.schema.yaml(1 hunks)specs/sdl/sdl-input.schema.yaml(1 hunks)specs/sdl/validation-limitations.md(1 hunks)testdata/sdl/invalid/credentials-missing-host.yaml(1 hunks)testdata/sdl/invalid/endpoint-not-used.yaml(1 hunks)testdata/sdl/invalid/invalid-port.yaml(1 hunks)testdata/sdl/invalid/missing-deployment.yaml(1 hunks)testdata/sdl/invalid/missing-image.yaml(1 hunks)testdata/sdl/invalid/missing-version.yaml(1 hunks)testdata/sdl/invalid/negative-cpu.yaml(1 hunks)testdata/sdl/invalid/persistent-without-mount.yaml(1 hunks)testdata/sdl/v2.0/gpu-basic/groups.json(1 hunks)testdata/sdl/v2.0/gpu-basic/input.yaml(1 hunks)testdata/sdl/v2.0/gpu-basic/manifest.json(1 hunks)testdata/sdl/v2.0/http-options/groups.json(1 hunks)testdata/sdl/v2.0/http-options/input.yaml(1 hunks)testdata/sdl/v2.0/http-options/manifest.json(1 hunks)testdata/sdl/v2.0/ip-endpoint/groups.json(1 hunks)testdata/sdl/v2.0/ip-endpoint/input.yaml(1 hunks)testdata/sdl/v2.0/ip-endpoint/manifest.json(1 hunks)testdata/sdl/v2.0/multiple-services/groups.json(1 hunks)testdata/sdl/v2.0/multiple-services/input.yaml(1 hunks)testdata/sdl/v2.0/multiple-services/manifest.json(1 hunks)testdata/sdl/v2.0/persistent-storage/groups.json(1 hunks)testdata/sdl/v2.0/persistent-storage/input.yaml(1 hunks)testdata/sdl/v2.0/persistent-storage/manifest.json(1 hunks)testdata/sdl/v2.0/simple/groups.json(1 hunks)testdata/sdl/v2.0/simple/input.yaml(1 hunks)testdata/sdl/v2.0/simple/manifest.json(1 hunks)testdata/sdl/v2.0/storage-classes/groups.json(1 hunks)testdata/sdl/v2.0/storage-classes/input.yaml(1 hunks)testdata/sdl/v2.0/storage-classes/manifest.json(1 hunks)testdata/sdl/v2.1/credentials/groups.json(1 hunks)testdata/sdl/v2.1/credentials/input.yaml(1 hunks)testdata/sdl/v2.1/credentials/manifest.json(1 hunks)testdata/sdl/v2.1/ip-endpoint/groups.json(1 hunks)testdata/sdl/v2.1/ip-endpoint/input.yaml(1 hunks)testdata/sdl/v2.1/ip-endpoint/manifest.json(1 hunks)ts/package.json(1 hunks)ts/src/sdl/SDL/SDL.ts(9 hunks)ts/src/sdl/SDL/parity.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- specs/sdl/validation-limitations.md
- testdata/sdl/invalid/negative-cpu.yaml
🚧 Files skipped from review as they are similar to previous changes (37)
- go/sdl/groupBuilder_v2.go
- .github/workflows/tests.yaml
- testdata/sdl/invalid/endpoint-not-used.yaml
- testdata/sdl/invalid/missing-version.yaml
- testdata/sdl/v2.0/persistent-storage/manifest.json
- go/sdl/parity_test.go
- make/test.mk
- testdata/sdl/v2.0/storage-classes/groups.json
- go/sdl/go.mod
- testdata/sdl/v2.0/multiple-services/groups.json
- testdata/sdl/v2.1/ip-endpoint/manifest.json
- testdata/sdl/v2.0/ip-endpoint/manifest.json
- testdata/sdl/v2.0/multiple-services/manifest.json
- testdata/sdl/v2.0/simple/groups.json
- testdata/sdl/v2.0/gpu-basic/groups.json
- testdata/sdl/invalid/missing-deployment.yaml
- testdata/sdl/v2.0/http-options/input.yaml
- testdata/sdl/v2.0/simple/manifest.json
- testdata/sdl/v2.0/http-options/groups.json
- testdata/sdl/invalid/missing-image.yaml
- testdata/sdl/v2.0/ip-endpoint/groups.json
- ts/package.json
- go/sdl/cmd/generate-sdl-fixtures/main.go
- testdata/sdl/v2.0/gpu-basic/manifest.json
- testdata/sdl/v2.0/storage-classes/manifest.json
- specs/sdl/manifest.schema.yaml
- testdata/sdl/v2.1/credentials/groups.json
- testdata/sdl/v2.0/simple/input.yaml
- testdata/sdl/invalid/credentials-missing-host.yaml
- testdata/sdl/v2.1/ip-endpoint/input.yaml
- testdata/sdl/v2.0/gpu-basic/input.yaml
- testdata/sdl/v2.1/ip-endpoint/groups.json
- testdata/sdl/invalid/persistent-without-mount.yaml
- testdata/sdl/v2.1/credentials/manifest.json
- specs/sdl/groups.schema.yaml
- .gitignore
- testdata/sdl/v2.0/http-options/manifest.json
🧰 Additional context used
🧬 Code graph analysis (1)
ts/src/sdl/SDL/SDL.ts (2)
ts/src/sdl/index.ts (1)
SdlValidationError(28-28)ts/src/sdl/types.ts (1)
v3Sdl(100-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: go
🔇 Additional comments (14)
testdata/sdl/v2.0/persistent-storage/input.yaml (1)
3-48: SDL configuration is well-formed and consistent.The YAML SDL v2.0 configuration correctly defines:
- Persistent storage setup with Postgres database service
- Compute profile with appropriate resource specifications (500m CPU, 1Gi RAM, 50Gi storage)
- Placement constraints (region us-west, signed_by policy)
- Pricing and deployment mappings
Cross-referenced against the companion
groups.jsonfixture, resource specifications are consistent.testdata/sdl/v2.0/persistent-storage/groups.json (1)
1-70: JSON fixture data is accurate and consistent with input.yaml.Byte conversions are correct:
- Memory: 1,073,741,824 bytes = 1 GiB ✓
- Storage: 53,687,091,200 bytes = 50 GiB ✓
All resource specifications (CPU 500 units, storage class/attributes, pricing, count) align with the corresponding input.yaml file. The data structure appears well-suited for parity test validation.
testdata/sdl/v2.0/storage-classes/input.yaml (2)
1-1: The schema reference path is correct and resolves properly. The filespecs/sdl/sdl-input.schema.yamlexists at the expected location, and the relative path../../../../specs/sdl/sdl-input.schema.yamlcorrectly references the SDL input schema from the file's location attestdata/sdl/v2.0/storage-classes/input.yaml.
2-58: LGTM! Comprehensive storage-classes fixture.The SDL v2.0 manifest provides a well-structured test fixture that exercises the storage-classes feature with parametric storage definitions, mixed storage attributes (persistent flags and class names), and proper deployment configuration. The mixed attribute patterns (cache/logs minimal vs. data with both persistent and class) are intentional for testing edge cases. The fixture validates successfully against the sdl-input.schema.yaml schema.
go/sdl/groupBuilder_v2_1.go (1)
115-121: LGTM! Deterministic ordering ensures parity testing reliability.The implementation correctly addresses non-deterministic Go map iteration by collecting storage names, sorting them, and iterating in sorted order. This ensures consistent output across runs, which is essential for the parity tests between Go and TypeScript implementations.
testdata/sdl/v2.0/ip-endpoint/input.yaml (1)
1-49: LGTM!Well-structured SDL v2.0 test fixture for IP endpoint functionality. The configuration correctly demonstrates:
- Dual port exposure (80 and 443) with IP endpoint references
- Proper endpoint declaration matching the IP references in the service expose section
- Valid compute profile and placement configuration
testdata/sdl/v2.0/multiple-services/input.yaml (1)
1-93: LGTM!Comprehensive multi-service SDL v2.0 test fixture demonstrating:
- Service dependency chains (web → api → db)
- Multiple compute profiles with varying resource allocations
- Both
anyOfandallOfsignedBy constraints- Inter-service expose configurations
The hardcoded credentials (lines 29, 18) are appropriate for test fixtures.
testdata/sdl/invalid/invalid-port.yaml (1)
1-32: LGTM!Valid negative test fixture with intentionally invalid port value (99999 > 65535). This tests the port validation logic in
validateServicePortswhich enforcesport > 0 && port <= 65535.testdata/sdl/v2.1/credentials/input.yaml (1)
1-43: LGTM!Well-structured SDL v2.1 test fixture for private registry credentials. Correctly demonstrates the credentials block structure (host, email, username, password) for private container registry authentication.
ts/src/sdl/SDL/SDL.ts (3)
113-122: LGTM!Good addition of version field validation in
fromString. This catches malformed SDLs early and provides a clear error message.
834-864: LGTM!Good addition of deterministic storage key ordering via
.sort()in bothv2ManifestServiceParamsandv3ManifestServiceParams. This ensures consistent output for parity testing between Go and TypeScript implementations.
899-917: LGTM!Clean refactor of params handling. The conditional assignment (
if (params !== null)) prevents null params from polluting the manifest output.ts/src/sdl/SDL/parity.spec.ts (2)
24-79: LGTM!
makeSchemaLenientappropriately relaxes JSON Schema constraints for flexible parity testing. The recursive handling of nested structures (properties,items,anyOf,oneOf,allOf) is thorough.
303-315: Verify: v2.0 fixtures usev3Manifestmethod.The tests call
sdl.v3Manifest(true)for v2.0/beta2 fixtures instead ofsdl.v2Manifest(). The normalization handles GPU filtering for beta2, but verify this is the intended parity comparison approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
specs/sdl/manifest.schema.yaml (1)
1-6: Consider adding standard JSON Schema root declarations.The schema starts directly with
items:without a root-leveltype: arraydeclaration or$schemaidentifier. While this may work with some validators, adding these improves compatibility and clarity:+$schema: "https://json-schema.org/draft/2020-12/schema" +type: array items: additionalProperties: false properties: name: type: stringts/src/sdl/SDL/parity.spec.ts (2)
68-76: Redundant processing for array items.Lines 68-76 re-check
additionalPropertiesandrequiredonitemsSchema, butmakeSchemaLenientwas already called onlenient.itemsat line 53, which would have handled these. This block is defensive but unnecessary.- if (lenient.type === "array" && lenient.items) { - const itemsSchema = lenient.items as Record<string, unknown>; - if (itemsSchema.additionalProperties === false) { - itemsSchema.additionalProperties = true; - } - if (itemsSchema.required && Array.isArray(itemsSchema.required)) { - delete itemsSchema.required; - } - } - return lenient;
81-101: Schema is recompiled on every validation call.
ajv.compile()is called each timevalidateAgainstSchemais invoked. Since the same schemas are validated multiple times across fixtures, consider caching compiled validators.+const schemaCache = new Map<string, ReturnType<typeof ajv.compile>>(); + function validateAgainstSchema(actual: unknown, schemaPath: string): { valid: boolean; errors: string[] } { if (!fs.existsSync(schemaPath)) { return { valid: true, errors: [] }; } + let validate = schemaCache.get(schemaPath); + if (!validate) { const schemaContent = fs.readFileSync(schemaPath, "utf8"); const schema = load(schemaContent) as JSONSchema; const lenientSchema = makeSchemaLenient(schema); - const validate = ajv.compile(lenientSchema as Record<string, unknown>); + validate = ajv.compile(lenientSchema as Record<string, unknown>); + schemaCache.set(schemaPath, validate); + } const valid = validate(actual);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
specs/sdl/manifest.schema.yaml(1 hunks)ts/src/sdl/SDL/parity.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: proto
- GitHub Check: go
- GitHub Check: sdl-parity
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (7)
specs/sdl/manifest.schema.yaml (2)
295-314: Good use of$defsfor reusable credentials schema.The credentials schema is now properly defined once in
$defsand referenced via$refat lines 26 and 105, eliminating the previous duplication.
33-95: LGTM!The
exposeschema is well-structured with proper nesting forhttpOptions, required fields, and nullable patterns usinganyOfwithtype: 'null'.ts/src/sdl/SDL/parity.spec.ts (5)
103-171: LGTM!The
normalizeKeysfunction correctly handles key transformations and value normalization. The previous issue withfinalValuebeing overwritten has been addressed—now onlyfinalValueis assigned tonormalized[normalizedKey].
179-188: Numeric string normalization may have edge cases.The logic at lines 181-186 only normalizes strings containing
.and checks for specific trailing zero patterns. Integer strings (e.g.,"100") won't be converted, and strings like"1.0"might not match the.000000000000000000pattern.If this is intentional to handle specific Go/TS output differences, consider adding a brief comment explaining the expected scenarios.
251-280: LGTM!The
loadFixturesfunction correctly discovers and validates fixture directories, ensuring only complete fixtures (with input.yaml, manifest.json, and groups.json) are returned.
402-411: Invalid fixtures only tested against beta3.The invalid fixtures test uses hardcoded
"beta3"version. If v2.0/beta2 has different validation rules, consider also testing invalid fixtures against beta2, or documenting that invalid fixtures are version-specific.
295-329: LGTM!The v2.0 test suite correctly loads fixtures, normalizes both actual and expected outputs, compares them, and validates against schemas. The error logging on validation failure aids debugging.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @go/sdl/schema_aggressive_test.go:
- Around line 115-146: The pricing test is shadowing the outer sdl variable by
using `sdl := fmt.Sprintf(...)` inside the else-if, causing later validation to
operate on the wrong variable; change the short declaration to an assignment so
the existing outer `sdl` is reused (i.e., replace `sdl := fmt.Sprintf(...)` with
`sdl = fmt.Sprintf(...)`) so `validateInputAgainstSchema([]byte(sdl))` and the
subsequent test flow use the intended constructed SDL (function reference:
validateInputAgainstSchema, variable: sdl, loop case: tt.field containing
"pricing:").
In @go/sdl/sdl-input.schema.yaml:
- Around line 274-421: The service object schema under "services" currently
defines properties (args, command, credentials, etc.) but is missing
additionalProperties: false; add additionalProperties: false to that service
object (the object that contains properties: args, command, credentials,
dependencies, env, expose, image, params) just before the required: block so
unknown fields at the service level are rejected consistent with nested schemas.
🧹 Nitpick comments (5)
go/sdl/sdl-input.schema.yaml (1)
178-185: CPU units pattern is unnecessarily complex.The pattern
^-?[0-9]+(\.[0-9]+)?[a-zA-Z]*$explicitly allows an optional minus sign, then immediately usesnot: { pattern: '^-' }to exclude it. This is redundant and confusing.A cleaner approach would be:
♻️ Suggested simplification
anyOf: - type: string - pattern: '^-?[0-9]+(\.[0-9]+)?[a-zA-Z]*$' - not: - pattern: '^-' + pattern: '^[0-9]+(\.[0-9]+)?[a-zA-Z]*$' - type: number minimum: 0go/sdl/schema_additional_props_test.go (1)
11-650: Comprehensive negative test coverage, consider adding a positive baseline.The test suite thoroughly covers unknown field rejection across 16 different SDL sections. However, adding at least one positive test case (valid SDL without unknown fields that should pass) would serve as a baseline sanity check to ensure the validator works correctly in the happy path.
♻️ Suggested addition
{ name: "valid SDL without unknown fields", sdl: `version: "2.0" services: web: image: nginx profiles: compute: web: resources: cpu: units: 1 memory: size: 1Gi storage: - size: 1Gi placement: dc: pricing: web: denom: uakt amount: 1 deployment: web: dc: profile: web count: 1 `, shouldErr: false, reason: "valid SDL should pass", },go/sdl/gpu_test.go (1)
156-159: Index-based attribute comparison may be order-dependent.The assertion
p.Attributes[i].Keyassumes attributes maintain a specific order. If the order of attributes changes in the implementation (e.g., map iteration in Go is non-deterministic), tests could fail spuriously.Consider using a map-based comparison or sorting attributes before comparison for robustness.
♻️ Alternative approach using map comparison
// Convert to map for order-independent comparison attrMap := make(map[string]string) for _, attr := range p.Attributes { attrMap[attr.Key] = attr.Value } for _, expected := range tt.expectedAttrs { require.Equal(t, expected.value, attrMap[expected.key], "attribute %s", expected.key) }go/sdl/schema_edge_cases_test.go (1)
147-149: Non-English comment should be translated.Lines 147-148 contain a comment in Ukrainian:
// Note: Go код використовує url.ParseRequestURI("http://" + item) // Це означає що валідація досить ліберальнаFor maintainability and consistency, translate to English:
♻️ Suggested translation
- // Note: Go код використовує url.ParseRequestURI("http://" + item) - // Це означає що валідація досить ліберальна + // Note: Go code uses url.ParseRequestURI("http://" + item) + // This means validation is fairly liberalgo/sdl/schema_complex_rules_test.go (1)
689-816: Test structure can be simplified.The
TestSchemaValidation_ExposeToServicetest uses a conditional based on test name to select SDL content (lines 709-806), which defeats the purpose of table-driven testing. Consider moving the SDL strings directly into the test table or using separate test functions.♻️ Suggested refactor
func TestSchemaValidation_ExposeToService(t *testing.T) { tests := []struct { name string sdl string shouldErr bool reason string }{ { name: "valid service routing", sdl: `version: "2.0" services: web: image: nginx expose: - port: 80 to: - service: api api: image: node:18 ...`, // full SDL here shouldErr: false, }, { name: "multiple to targets", sdl: `version: "2.0" ...`, // full SDL here shouldErr: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { err := validateInputAgainstSchema([]byte(tt.sdl)) // assertions }) } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
go/sdl/gpu_test.gogo/sdl/schema_additional_props_test.gogo/sdl/schema_aggressive_test.gogo/sdl/schema_complex_rules_test.gogo/sdl/schema_edge_cases_test.gogo/sdl/schema_validation_test.gogo/sdl/sdl-input.schema.yamlgo/sdl/storage_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: sdl-parity
- GitHub Check: test
- GitHub Check: coverage
- GitHub Check: proto
- GitHub Check: go
🔇 Additional comments (14)
go/sdl/sdl-input.schema.yaml (2)
144-155: Endpoint name pattern requires minimum 2 characters.The pattern
^[a-z]+[-_0-9a-z]+$requires at least one lowercase letter followed by at least one character from[-_0-9a-z]. This means single-character endpoint names (e.g.,a) would be rejected by the schema.Verify this is the intended behavior. If single-character names should be allowed:
♻️ Alternative pattern for single-char support
patternProperties: - '^[a-z]+[-_0-9a-z]+$': + '^[a-z][-_0-9a-z]*$':
1-9: Well-structured SDL schema with clear documentation.The schema provides comprehensive structural validation with:
- Clear separation of definitions for reusable types
- Conditional validation for storage classes (RAM vs persistent)
- Appropriate constraints for HTTP options, ports, and pricing
The note about semantic validations being handled at runtime is helpful for maintainers.
go/sdl/gpu_test.go (1)
10-162: Well-structured table-driven GPU tests.The refactored test consolidates multiple scenarios into a clean, maintainable structure with clear test case definitions and proper error handling separation.
go/sdl/storage_test.go (3)
15-74: Clean table-driven storage parsing tests.The refactored
TestStorage_Parseconsolidates storage parsing scenarios with appropriatecheckFunccallbacks for custom assertions. The structure properly separates error cases from success cases.
76-185: Comprehensive storage attribute validation tests.The
TestStorage_Attributestest covers important edge cases including:
- Unknown attributes rejection
- Duplicate volume names
- Ephemeral vs persistent storage class rules
- RAM class constraints
The use of
errTypefor specific error matching strengthens test reliability.
302-387: Schema class validation tests are well-structured.The
TestStorage_SchemaValidation_Classestests properly verify the conditional schema rules for RAM vs non-RAM storage classes with clear descriptions for each case.go/sdl/schema_edge_cases_test.go (2)
10-122: Thorough HTTP options limit testing.The test covers important boundary conditions for
max_body_size,read_timeout, andsend_timeoutincluding exact limits (100 MB, 60000 ms) and rejection of values exceeding limits. Negative value tests are also included.
363-393: Empty image validation test is concise and effective.Simple, focused test that validates a critical schema constraint.
go/sdl/schema_validation_test.go (3)
10-98: Credentials validation tests are comprehensive.Tests cover required fields, minimum length constraints, and empty value rejection. Good coverage of the
credentialsschema definition.
328-393: Endpoint name pattern tests align with schema.The tests correctly validate the pattern
^[a-z]+[-_0-9a-z]+$from the schema, covering valid names (lowercase, dashes, underscores, numbers) and invalid patterns (uppercase, starting with numbers/special chars, spaces, dots).
449-592: Required fields tests provide good coverage.Tests validate that the schema enforces required fields at multiple levels: top-level (version, services, profiles, deployment), service-level (image), and storage-level (size).
go/sdl/schema_complex_rules_test.go (3)
10-100: Next cases 'off' logic tests validate complex schema rule.Good coverage of the schema rule that
offcan only appear alone in thenext_casesarray, not combined with other error codes.
311-405: IP requires global validation is thorough.Tests cover the conditional schema rule requiring
global: truewhenipis specified, including edge cases like empty IP strings and absence of global field.
818-927: Storage params validation tests are well-structured.Tests cover various scenarios for storage params including mount paths, readOnly boolean constraints, and optional fields. The comment on line 859 correctly notes that cross-reference validation is not handled by schema.
| services: | ||
| additionalProperties: | ||
| properties: | ||
| args: | ||
| $ref: '#/definitions/stringArrayOrNull' | ||
| command: | ||
| $ref: '#/definitions/stringArrayOrNull' | ||
| credentials: | ||
| additionalProperties: false | ||
| properties: | ||
| email: | ||
| type: string | ||
| minLength: 5 | ||
| host: | ||
| type: string | ||
| minLength: 1 | ||
| password: | ||
| type: string | ||
| minLength: 6 | ||
| username: | ||
| type: string | ||
| minLength: 1 | ||
| required: | ||
| - host | ||
| - username | ||
| - password | ||
| type: object | ||
| dependencies: | ||
| items: | ||
| additionalProperties: false | ||
| properties: | ||
| service: | ||
| type: string | ||
| type: object | ||
| type: array | ||
| env: | ||
| $ref: '#/definitions/stringArrayOrNull' | ||
| expose: | ||
| items: | ||
| additionalProperties: false | ||
| properties: | ||
| accept: | ||
| items: | ||
| type: string | ||
| type: array | ||
| as: | ||
| $ref: '#/definitions/portNumber' | ||
| http_options: | ||
| additionalProperties: false | ||
| properties: | ||
| max_body_size: | ||
| type: integer | ||
| minimum: 0 | ||
| maximum: 104857600 | ||
| description: Maximum body size in bytes (max 100 MB) | ||
| next_cases: | ||
| anyOf: | ||
| - type: array | ||
| items: | ||
| $ref: '#/definitions/httpErrorCode' | ||
| contains: | ||
| const: off | ||
| maxItems: 1 | ||
| minItems: 1 | ||
| - type: array | ||
| items: | ||
| $ref: '#/definitions/httpErrorCode' | ||
| not: | ||
| contains: | ||
| const: off | ||
| next_timeout: | ||
| type: integer | ||
| minimum: 0 | ||
| next_tries: | ||
| type: integer | ||
| minimum: 0 | ||
| read_timeout: | ||
| type: integer | ||
| minimum: 0 | ||
| maximum: 60000 | ||
| description: Read timeout in milliseconds (max 60 seconds) | ||
| send_timeout: | ||
| type: integer | ||
| minimum: 0 | ||
| maximum: 60000 | ||
| description: Send timeout in milliseconds (max 60 seconds) | ||
| type: object | ||
| port: | ||
| $ref: '#/definitions/portNumber' | ||
| proto: | ||
| enum: | ||
| - TCP | ||
| - UDP | ||
| - tcp | ||
| - udp | ||
| type: string | ||
| to: | ||
| items: | ||
| additionalProperties: false | ||
| properties: | ||
| global: | ||
| type: boolean | ||
| ip: | ||
| minLength: 1 | ||
| type: string | ||
| service: | ||
| type: string | ||
| allOf: | ||
| - if: | ||
| properties: | ||
| ip: | ||
| type: string | ||
| minLength: 1 | ||
| required: | ||
| - ip | ||
| then: | ||
| properties: | ||
| global: | ||
| const: true | ||
| required: | ||
| - global | ||
| type: object | ||
| type: array | ||
| required: | ||
| - port | ||
| type: object | ||
| type: array | ||
| image: | ||
| type: string | ||
| minLength: 1 | ||
| params: | ||
| additionalProperties: false | ||
| properties: | ||
| storage: | ||
| additionalProperties: | ||
| additionalProperties: false | ||
| properties: | ||
| mount: | ||
| $ref: '#/definitions/absolutePath' | ||
| readOnly: | ||
| type: boolean | ||
| type: object | ||
| type: object | ||
| type: object | ||
| required: | ||
| - image | ||
| type: object | ||
| type: object |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the schema file at the specified lines
cd go/sdl
head -421 sdl-input.schema.yaml | tail -148 | cat -nRepository: akash-network/chain-sdk
Length of output: 5574
🏁 Script executed:
# Check all occurrences of additionalProperties in the schema
rg -n "additionalProperties" go/sdl/sdl-input.schema.yamlRepository: akash-network/chain-sdk
Length of output: 1414
🏁 Script executed:
# Search for how the Go parser handles unknown or additional fields
find go/sdl -name "*.go" -type f | head -20Repository: akash-network/chain-sdk
Length of output: 423
🏁 Script executed:
# Look for schema validation or field parsing logic
rg -n "additionalProperties|Unknown|unknown.*field" go/sdl/ --type goRepository: akash-network/chain-sdk
Length of output: 5118
🏁 Script executed:
# Check if there's a test for unknown fields in services
rg -A 30 "unknown field in.*service" go/sdl/schema_additional_props_test.goRepository: akash-network/chain-sdk
Length of output: 49
🏁 Script executed:
# Get the full context around services in the test file
rg -B 5 -A 15 "services" go/sdl/schema_additional_props_test.goRepository: akash-network/chain-sdk
Length of output: 6478
🏁 Script executed:
# Check what the schema structure looks like more carefully - services property definition
sed -n '275,280p' go/sdl/sdl-input.schema.yamlRepository: akash-network/chain-sdk
Length of output: 241
🏁 Script executed:
# Look for any documentation or comments about services allowing additional properties
rg -B 3 -A 3 "services:" go/sdl/sdl-input.schema.yaml | head -30Repository: akash-network/chain-sdk
Length of output: 174
Add additionalProperties: false to the service object schema for consistency.
The services object schema (lines 275-420) lacks additionalProperties: false on the service object itself. All nested structures—credentials, dependencies, expose items, http_options, params, and others—explicitly use additionalProperties: false to reject unknown fields. The service object should follow the same pattern to prevent accidental typos (e.g., typo_field: value) from silently being ignored.
This inconsistency is confirmed by the absence of any test case for unknown fields at the service level in schema_additional_props_test.go, while comprehensive tests exist for all other structures. Add additionalProperties: false after line 420 (before required: - image) to align with the schema's validation strategy.
🤖 Prompt for AI Agents
In @go/sdl/sdl-input.schema.yaml around lines 274 - 421, The service object
schema under "services" currently defines properties (args, command,
credentials, etc.) but is missing additionalProperties: false; add
additionalProperties: false to that service object (the object that contains
properties: args, command, credentials, dependencies, env, expose, image,
params) just before the required: block so unknown fields at the service level
are rejected consistent with nested schemas.
| persistent: | ||
| anyOf: | ||
| - const: true | ||
| - const: "true" |
There was a problem hiding this comment.
suggestion: this can be expressed in simpler way with not:
not:
properties:
class: { const: ram }
persistent:
anyOf:
- { const: true }
- { const: "true" }
required: [class, persistent]
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
go/sdl/sdl-input.schema.yaml (3)
199-201: Consider adding email format validation.The
minLength: 5, which would accept invalid strings like "aaaaa". JSON Schema draft-07 supportsformat: emailfor basic validation.Suggested improvement
email: type: string minLength: 5 + format: email
301-307: Simplify CPU units pattern by removing redundant negative handling.The pattern allows optional
-prefix (^-?...) but then usesnot: pattern: '^-'to reject it. This is correct but convoluted. Consider simplifying by removing-?from the base pattern.Simplified pattern
units: oneOf: - type: string - pattern: '^-?[0-9]+(\.[0-9]+)?[a-zA-Z]*$' - not: - pattern: '^-' + pattern: '^[0-9]+(\.[0-9]+)?[a-zA-Z]*$' - type: number minimum: 0
411-422: Property names shadow JSON Schema keywords.
allOfandanyOfare JSON Schema keywords but are used here as property names withinsignedBy. This is valid but could confuse maintainers reading the schema. Consider adding a brief inline comment.signedBy: additionalProperties: false + description: Signing requirements (property names allOf/anyOf are domain-specific, not JSON Schema keywords) properties: allOf:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go/cli/go.sumis excluded by!**/*.sumgo/sdl/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go/cli/go.modgo/sdl/go.modgo/sdl/groupBuilder_v2.gogo/sdl/groupBuilder_v2_1.gogo/sdl/schema_validation_test.gogo/sdl/sdl-input.schema.yamlgo/sdl/sdl.go
🚧 Files skipped from review as they are similar to previous changes (3)
- go/sdl/groupBuilder_v2.go
- go/sdl/sdl.go
- go/cli/go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: go
- GitHub Check: sdl-parity
- GitHub Check: test
- GitHub Check: coverage
- GitHub Check: proto
🔇 Additional comments (10)
go/sdl/groupBuilder_v2_1.go (1)
117-129: LGTM! Deterministic storage ordering is correct and essential for parity tests.The implementation follows the standard Go pattern for deterministic map iteration: collect keys, sort, then iterate. This ensures the manifest output is reproducible and comparable across Go and TypeScript parsers.
go/sdl/sdl-input.schema.yaml (3)
264-277: Verify endpoint name minimum length requirement.The pattern
'^[a-z]+[-_0-9a-z]+$'requires at least 2 characters (one[a-z]followed by at least one[-_0-9a-z]). Single-character endpoint names like"a"would be rejected. Verify this is intentional.If single-character names should be valid, consider:
Alternative pattern allowing single-character names
patternProperties: - '^[a-z]+[-_0-9a-z]+$': + '^[a-z][-_0-9a-z]*$':
432-475: MissingadditionalProperties: falseon service definitions.Unlike other objects in this schema (e.g.,
credentials,exposeItem,httpOptions), service definitions don't restrict additional properties. This allows arbitrary unknown fields on services, which may be intentional for extensibility but is inconsistent with the schema's otherwise strict validation approach.Add restriction if strict validation is desired
services: additionalProperties: properties: args: $ref: '#/definitions/stringArrayOrNull' # ... other properties ... required: - image type: object + additionalProperties: false type: object
1-9: Well-structured schema with comprehensive validation.The schema provides thorough structural validation with good use of JSON Schema features like conditional validation (
allOf/if/then),oneOffor polymorphic types, andpatternProperties. The documentation clearly states that semantic validations are handled at runtime by the Go parser.go/sdl/schema_validation_test.go (5)
11-98: Well-structured table-driven test for credential validation.Good coverage of credential field constraints including required fields (host), minimum length validation (email, password), and empty string rejection (username).
100-447: Comprehensive validation tests for ports, protocols, denoms, CPU units, endpoints, and versions.The tests provide excellent boundary coverage (ports 1, 65535, 0, 65536), case sensitivity handling (TCP/tcp), format variations (CPU milli units, IBC denoms), and enum validation. The version test at Line 407 correctly validates that unquoted
2.0(parsed as float by YAML) is rejected.
449-592: Good coverage of required field validation.The tests comprehensively verify that missing required fields (version, deployment, image, storage size, services, profiles) are correctly rejected by the schema. All test cases appropriately expect validation errors.
694-784: Excellent complementary test for GPU attribute validation.This test correctly validates the inverse rule: GPU attributes require units > 0. Together with
TestSchemaValidation_GPUUnitsRequireAttributes, these tests provide complete coverage of the bidirectional GPU validation requirement.
594-692: GPU units validation correctly tests bidirectional requirement between units > 0 and attributes.The tests properly validate the schema rules:
- GPU units > 0 requires attributes
- Empty GPU (
gpu: {}) is valid because units defaults to 0 per the schema specification- All edge cases (string vs numeric units, zero values, decimal units) are covered appropriately
The test case at line 647-651 correctly documents that the schema allows empty GPU sections, explicitly defined in the schema description: "units defaults to 0 if omitted."
go/sdl/go.mod (1)
8-8: Dependencies are appropriate and secure.The
gojsonschemav1.2.0 library and its transitive dependencies (gojsonpointer,gojsonreference) align with the new schema validation capability. The library is current (latest release) with no known security vulnerabilities. The promotion ofcosmossdk.io/logto a direct dependency indicates it is now directly used in the schema validation code.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @go/sdl/schema_aggressive_test.go:
- Around line 116-145: The test creates a new shadowed sdl variable with :=
inside the subtest; change that to = so it assigns to the outer sdl (use sdl =
fmt.Sprintf(..., tt.field)), then remove the duplicate call to
validateInputAgainstSchema(...) and the immediate return so the test falls
through to the shared validation logic that calls validateInputAgainstSchema and
asserts tt.shouldErr/tt.shouldNotErr (refer to the existing
validateInputAgainstSchema usage and tt.shouldErr/tt.reason symbols).
In @go/sdl/sdl-input.schema.yaml:
- Around line 432-476: The services' per-service object schema currently allows
unknown keys because the object under services -> additionalProperties (the
schema that defines each service) is missing additionalProperties: false; update
that object schema (the node that contains properties like image, args, command,
env, params, credentials, dependencies, expose and the existing required:
[image] and type: object) to include additionalProperties: false so unknown/typo
properties are rejected during validation.
🧹 Nitpick comments (3)
go/sdl/schema_aggressive_test.go (1)
633-727: Consider adding a fallback for unhandled field values.If the
fieldvalue doesn't match"VERSION","denom", or"proto", thesdlvariable remains empty and validation runs on an empty string. While current test cases all match, this could cause confusing failures if new cases are added.Optional: Add a test failure for unhandled cases
} else if tt.field == "proto" { sdl = fmt.Sprintf(`version: "2.0" ... - } + } else { + t.Fatalf("unhandled field type: %s", tt.field) + } err := validateInputAgainstSchema([]byte(sdl))go/sdl/sdl-input.schema.yaml (2)
195-215: Consider adding email format validation.The
minLength: 5but no format validation. JSON Schema draft-07 supports"format": "email"for basic email validation. This could catch obvious typos early.🔧 Optional: Add email format validation
email: type: string minLength: 5 + format: email
348-377: Minor: Zero-value edge cases in GPU units string validation.The string validation excludes
"0"and"0.0"but edge cases like"00","0.00", or"0.000"would pass schema validation while still representing zero. The Go parser likely catches these, but a pattern like^0+(\.0+)?$in anotclause would be more robust.🔧 Optional: More robust zero detection for GPU units
- type: string not: - enum: ["0", "0.0"] + pattern: '^0+(\.0+)?$'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
go/sdl/schema_aggressive_test.gogo/sdl/sdl-input.schema.yamlmake/test.mk
🚧 Files skipped from review as they are similar to previous changes (1)
- make/test.mk
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: proto
- GitHub Check: sdl-parity
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: go
🔇 Additional comments (15)
go/sdl/schema_aggressive_test.go (8)
1-9: LGTM!Package declaration and imports are appropriate for the test file.
180-294: LGTM!Well-structured table-driven tests covering special characters in service names, images, and domain accept lists. The test cases and assertions are clear.
296-438: LGTM!Comprehensive coverage of null and empty value handling. The comments clearly distinguish between schema validation and Go parser behavior.
440-527: LGTM!Good edge case tests for duplicate YAML keys. The comments correctly note this is YAML parser behavior.
529-592: LGTM!Clean table-driven tests for whitespace handling in image names.
731-768: LGTM!Clear test verifying the IP endpoint global requirement validation.
770-838: LGTM!Good coverage of storage name edge cases with clear comments about schema vs Go validation responsibilities.
840-968: LGTM!Comprehensive zero value testing across different SDL fields. The branching logic correctly handles all defined test cases.
go/sdl/sdl-input.schema.yaml (7)
1-8: LGTM!Clear schema metadata with appropriate draft version and helpful documentation explaining the schema's scope and limitations.
10-58: LGTM!Well-defined base types with correct port range validation (1-65535) and flexible price coin support for both native and IBC denominations.
153-193: LGTM!Well-designed
httpOptionswith proper bounds validation and clever handling of the mutually exclusiveoffvalue innext_casesusingcontains/notconstraints.
295-310: LGTM!CPU resource validation correctly requires
unitsand supports both numeric values and unit suffixes (e.g.,"1000m"for millicpu). The same minor zero-edge-case observation as GPU applies here.
479-492: LGTM!Version enum correctly restricts to supported SDL versions (2.0, 2.1) and required fields cover all essential SDL sections.
60-102: The Go parser validates unknown storage class values and rejects them. ThevalidateAttributeStorageClassfunction (go/sdl/storage.go:72-78) checks against an explicit whitelist of allowed classes (default,beta1,beta2,beta3,ram) and returns an error for any unknown value: "sdl: invalid value for attribute class". This validation occurs during YAML unmarshaling, so unknown classes do not reach downstream processing.Likely an incorrect or invalid review comment.
264-277: The 2-character minimum endpoint name constraint is intentionally enforced.The pattern
^[a-z]+[-_0-9a-z]+$requiring at least 2 characters is consistently implemented in both the schema (line 267) and the Go code validation regex (v2.go,v2_1.go,v2_2.go). All test cases and real examples use endpoint names with 2 or more characters. While single-character names are not explicitly tested, the constraint is deliberate and applied uniformly across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
go/sdl/schema_aggressive_test.go (4)
139-145: Minor inconsistency: pricing branch has inline validation while others fall through.The
pricing:case validates and returns early (lines 139-145), while other branches fall through to the common validation at lines 170-175. This works correctly but makes the control flow harder to follow.Consider extracting validation to a helper or restructuring all branches to fall through consistently.
440-527: Test names may be slightly misleading.These tests verify that YAML v3 parser rejects duplicate keys before schema validation occurs. The test function name
TestSchemaValidation_DuplicateNamessuggests schema validation, but the actual rejection happens at YAML parsing level.This is functionally correct since
validateInputAgainstSchemaincludes YAML parsing. Consider adding a brief comment at the function level clarifying this tests the full validation pipeline including YAML parsing.
633-719: Missing default case could cause confusing failures.If a new test case is added with a
fieldvalue not matching"VERSION","denom", or"proto", thesdlvariable remains empty, leading to a confusing validation error instead of a clear test setup failure.Suggested improvement
} else if tt.field == "proto" { sdl = fmt.Sprintf(`version: "2.0" ... profile: web count: 1`, tt.field, tt.value) + } else { + t.Fatalf("unhandled field type: %s", tt.field) }
872-958: Consider adding a fallback for unmatched field patterns.Similar to
TestSchemaValidation_CaseSensitivity, the if-else chain relies onstrings.Containsmatching. If a new test case is added with a field string that doesn't contain "amount:", "next_tries:", "cpu:", or "gpu:", it falls to the else branch which constructs SDL for cpu/gpu resources, potentially producing unexpected test behavior.This is low risk for current test cases but could cause subtle issues when extending tests.
go/sdl/storage_test.go (2)
46-53: Consider validating both attributes in the checkFunc.The test asserts there are 2 attributes but only validates the first one (
class: default). Consider also verifying the second attribute to ensure thepersistentattribute is correctly handled.Suggested improvement
checkFunc: func(t *testing.T, p v2ResourceStorageArray) { require.Len(t, p, 1) require.Equal(t, byteQuantity(1*unit.Gi), p[0].Quantity) require.Len(t, p[0].Attributes, 2) attr := types.Attributes(p[0].Attributes) require.Equal(t, "class", attr[0].Key) require.Equal(t, "default", attr[0].Value) + require.Equal(t, "persistent", attr[1].Key) + require.Equal(t, "true", attr[1].Value) },
301-343: Well-designed schema validation tests.Good coverage of the class/persistent attribute combinations. The
descriptionfield is helpful for test failure diagnostics.Consider adding edge cases for completeness:
- Storage class without explicit
persistentattribute (to test schema default handling)ramclass with nopersistentattribute at allgo/sdl/schema_complex_rules_test.go (1)
698-825: Test lacks rejection scenarios and uses fragile conditional pattern.
TestSchemaValidation_ExposeToServiceonly tests valid cases (shouldErr: false). The conditionalif tt.name == "valid service routing"pattern is fragile compared to embedding SDL in the test table. Consider:
- Adding error cases (e.g., invalid
tostructure)- Embedding SDL directly in the test struct for consistency with other tests
♻️ Suggested refactor
tests := []struct { name string + sdl string shouldErr bool reason string }{ { name: "valid service routing", + sdl: `version: "2.0" +services: + web: + image: nginx + expose: + - port: 80 + to: + - service: api +...`, shouldErr: false, }, + { + name: "invalid to item type", + sdl: `...`, + shouldErr: true, + reason: "to items must be objects", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - var sdl string - if tt.name == "valid service routing" { - sdl = `...` - } else { - sdl = `...` - } - - err := validateInputAgainstSchema([]byte(sdl)) + err := validateInputAgainstSchema([]byte(tt.sdl))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
go/sdl/schema_additional_props_test.gogo/sdl/schema_aggressive_test.gogo/sdl/schema_complex_rules_test.gogo/sdl/sdl-input.schema.yamlgo/sdl/storage_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: sdl-parity
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: proto
- GitHub Check: go
🔇 Additional comments (14)
go/sdl/schema_aggressive_test.go (5)
180-294: LGTM!The test structure is clean with complete SDL strings for each case, making test cases self-contained and easy to understand.
296-438: LGTM!Good test coverage of null/empty edge cases with helpful comments explaining the distinction between schema validation and Go parser validation.
529-592: LGTM!Good coverage of whitespace edge cases with clear documentation of schema vs runtime validation boundaries.
731-768: LGTM!Clear, focused test validating the IP endpoint + global constraint.
770-838: LGTM!Good documentation of schema vs Go parser validation boundaries for storage names.
go/sdl/storage_test.go (2)
75-184: LGTM!Well-structured table-driven test with comprehensive coverage of storage attribute validation scenarios. The error type checking pattern using
ErrorIswhenerrType != nilis appropriate for targeted error verification.
242-299: LGTM!The schema validation test for mount paths is well-structured. The template approach keeps tests DRY, and the test cases cover the essential validation scenarios: valid absolute paths, invalid relative paths, and empty strings.
go/sdl/schema_additional_props_test.go (1)
11-682: Comprehensive test coverage for additionalProperties constraints.The table-driven tests thoroughly cover unknown field rejection across all major SDL sections: services, credentials, dependencies, expose, http_options, CPU/memory/GPU/storage resources, endpoints, placement, pricing, and deployment. The assertion that errors contain "Additional property" ensures schema validation provides actionable error messages.
go/sdl/sdl-input.schema.yaml (4)
40-58: Price amount pattern allows zero values.The pattern
'^[0-9]+(\.[0-9]+)?$'matches "0" and "0.0". For the number type,minimum: 0also allows zero. If zero pricing is not semantically valid, consider adding exclusion:pattern: '^[1-9][0-9]*(\.[0-9]+)?$|^0\.[0-9]*[1-9][0-9]*$'If zero is intentionally allowed (e.g., for testing or special cases), this is fine as-is.
306-323: Consider whethernvidia: nullshould be valid.The schema allows
nvidia: nullviaoneOf: [array, 'null']. Combined withminProperties: 1on vendor, this permits:gpu: units: 1 attributes: vendor: nvidia: null # Satisfies minProperties but is semantically emptyIf this should be rejected at schema level, remove the null option:
nvidia: - oneOf: - - type: array - items: - ... - - type: 'null' + type: array + items: + ...Otherwise, ensure the Go parser handles this semantic case.
1-9: Well-structured schema with comprehensive validation.The schema effectively defines SDL input validation using JSON Schema draft-07. The description correctly notes that semantic validations (cross-references, unused endpoints, profile references) are handled by the Go parser at runtime, making this a clean separation of structural vs. semantic validation.
245-258: The endpoint name pattern is documented and intentional. The^[a-z]+[-_0-9a-z]+$pattern is explicitly listed inspecs/sdl/README.mdas a validation constraint for endpoint names, and all test fixtures consistently use multi-character names (endpoint1, endpoint2, myendpoint, myip, meow). No single-character endpoint names exist in the codebase, indicating the two-character minimum is by design to ensure meaningful endpoint identifiers.Likely an incorrect or invalid review comment.
go/sdl/schema_complex_rules_test.go (2)
1-100: Good coverage of complex schema validation rules.The tests effectively validate intricate schema constraints:
next_casesoff logic,signedBystructures, GPU bidirectional validation, IP/global requirements, dependencies, endpoint kinds, env/args/command arrays, and storage params. The table-driven pattern with clearreasonfields makes test intent explicit.
50-54: Empty array[]fornext_casesis correctly validated.The test expectation is accurate. An empty array satisfies the schema's second
oneOfoption (not: {contains: {const: off}}), which allows any array that doesn't contain "off"—including empty arrays. The test passes as expected.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @ts/src/sdl/SDL/SDL.ts:
- Around line 596-600: The trailing whitespace at the end of the line that
declares "const sortedIps = Array.from(ips).sort();" should be removed; edit the
line containing the "sortedIps" declaration (the line that uses
Array.from(ips).sort()) to eliminate any trailing spaces or tabs so there is no
trailing whitespace before the return that builds
Object.fromEntries(sortedIps.map(...)).
- Around line 780-793: The new group initialization is mis-indented and must be
placed inside the existing if (!group) block; move the group = { dgroup: { name:
placementName, resources: [], requirements: { attributes: attributes, signed_by:
{ all_of: infra.signedBy?.allOf && infra.signedBy.allOf.length > 0 ?
infra.signedBy.allOf : null, any_of: infra.signedBy?.anyOf &&
infra.signedBy.anyOf.length > 0 ? infra.signedBy.anyOf : null, }, }, },
boundComputes: {}, }; statement so it is indented and scoped under the if
(!group) branch (referencing the group variable, dgroup key, placementName,
attributes, and infra.signedBy) to fix the static analysis indentation error.
- Around line 168-178: Both resourceUnit and resourceValue have an unused
asString parameter; remove the unused parameter from both method signatures
(resourceUnit(val: string) and resourceValue(value: { toString: () => string } |
null)) and update their implementations to return the same values (resourceUnit
returns { val: convertResourceString(val) } and resourceValue returns value ===
null ? null : value.toString()), then update all call sites to stop passing the
asString argument wherever resourceUnit or resourceValue are invoked.
🧹 Nitpick comments (5)
go/sdl/schema_validation_test.go (2)
11-98: Good table-driven test coverage for credentials validation.The test cases cover the key validation scenarios. One optional enhancement: consider asserting on the error message content (e.g., using
require.ErrorContains) to ensure validation fails for the expected reason rather than an unrelated schema error.
456-599: Consider adding a positive test case for completeness.All test cases in this function expect errors. While other test functions cover valid SDL inputs, adding a single positive case (valid minimal SDL that passes) would strengthen this specific test function and guard against a schema that accidentally rejects everything.
Example positive test case
+ { + name: "valid minimal SDL", + sdl: `version: "2.0" +services: + web: + image: nginx +profiles: + compute: + web: + resources: + cpu: + units: 1 + memory: + size: 1Gi + storage: + - size: 1Gi + placement: + dc: + pricing: + web: + denom: uakt + amount: 1 +deployment: + web: + dc: + profile: web + count: 1`, + shouldErr: false, + reason: "minimal valid SDL should pass", + }, }ts/src/sdl/SDL/parity.spec.ts (1)
13-14: Consider simplifying AJV instantiation.The current cast is complex and uses
@ts-expect-error. A cleaner approach:♻️ Suggested simplification
-// @ts-expect-error - AjvModule has non-standard export, cast needed for instantiation -const ajv: { compile: (schema: Record<string, unknown>) => ValidateFunction } = new (AjvModule as unknown as new (options?: { allErrors?: boolean }) => typeof AjvModule)({ allErrors: true }); +const Ajv = AjvModule.default || AjvModule; +const ajv = new Ajv({ allErrors: true });go/sdl/sdl-input.schema.yaml (2)
410-421: Note:allOfandanyOfused as property names (not schema keywords).Lines 413-420 use
allOfandanyOfas property names within thesignedByobject definition. This is valid—they're just property names in this context, not JSON Schema composition keywords. However, it could cause confusion when reading the schema.Consider adding a brief inline comment clarifying these are Akash SDL field names, not schema constructs.
263-276: The 2-character minimum for endpoint names appears intentional and consistently enforced.The regex pattern
^[a-z]+[-_0-9a-z]+$does require at least 2 characters by design—enforced identically in both the schema and Go implementation (endpointNameValidationRegexingo/sdl/v2.go). The test suite (schema_validation_test.go) confirms valid names like "myendpoint", "my-endpoint", etc., with no test coverage for single-character names, suggesting the 2-character minimum is intentional.If single-character endpoint names need to be supported, update both the schema pattern and the Go regex to
^[a-z][-_0-9a-z]*$. Otherwise, no changes are required.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ts/package-lock.jsonis excluded by!**/package-lock.jsonts/src/sdl/SDL/__snapshots__/SDL.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
go/sdl/schema_validation_test.gogo/sdl/sdl-input.schema.yamlts/src/sdl/SDL/SDL.spec.tsts/src/sdl/SDL/SDL.tsts/src/sdl/SDL/parity.spec.tsts/src/sdl/types.ts
🧰 Additional context used
🧬 Code graph analysis (2)
ts/src/sdl/SDL/SDL.ts (2)
ts/src/sdl/sizes.ts (2)
convertResourceString(12-20)convertCpuResourceString(30-38)ts/src/sdl/types.ts (2)
v2ComputeResources(212-216)v3ComputeResources(218-224)
ts/src/sdl/SDL/parity.spec.ts (1)
ts/src/sdl/SDL/SDL.ts (3)
SDL(86-966)manifest(570-572)groups(737-739)
🪛 GitHub Check: ts
ts/src/sdl/SDL/SDL.ts
[failure] 172-172:
'asString' is defined but never used
[failure] 597-597:
Trailing spaces not allowed
[failure] 785-785:
Expected indentation of 16 spaces but found 14
[failure] 784-784:
Expected indentation of 14 spaces but found 12
[failure] 783-783:
Expected indentation of 14 spaces but found 12
[failure] 782-782:
Expected indentation of 14 spaces but found 12
[failure] 781-781:
Expected indentation of 12 spaces but found 10
[failure] 780-780:
Expected indentation of 10 spaces but found 8
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test
- GitHub Check: coverage
- GitHub Check: sdl-parity
- GitHub Check: proto
- GitHub Check: go
🔇 Additional comments (24)
go/sdl/schema_validation_test.go (9)
1-8: LGTM!Standard imports for table-driven tests using testify.
100-156: LGTM!Excellent boundary value testing for port validation covering min (1), max (65535), and invalid boundaries (0, 65536, -1).
158-216: LGTM!Good coverage testing case-insensitive TCP/UDP acceptance and rejection of unsupported protocols.
218-271: LGTM!Thorough testing of denom pattern validation including the IBC format variations.
273-333: LGTM!Comprehensive testing of CPU unit formats including milli notation and thorough zero-value coverage across different representations.
335-400: LGTM!Good coverage of endpoint naming pattern validation including edge cases for character position restrictions.
402-454: LGTM!Good version enum validation including the important edge case of unquoted numbers (line 414) which YAML parses as floats rather than strings.
601-713: LGTM!Excellent coverage of the conditional validation logic where GPU units > 0 requires attributes. Good handling of various zero representations (
0,"0","00","0.00").
715-805: LGTM!Good complementary test to the previous function, validating the inverse constraint that GPU attributes require units > 0.
ts/src/sdl/SDL/SDL.spec.ts (3)
614-677: LGTM! Test expectations updated for Go parity.The changes align the expected output with the Go parser's format:
signedBy.allOf: []→allOf: nullfor empty arraysmemory.quantity→memory.sizefield renaming- Added
gpublock withunits.val: "0"for consistency- Price amount precision increased to 18 decimal places
These changes are consistent with the type definitions in
types.tsand the SDL.ts implementation changes.
1320-1379: Test expectations consistent with earlier changes.The "SDL: Basic" test expectations follow the same pattern as the Minesweeper test, correctly updating
signedBy,memory.size,storage.size, GPU blocks, and price precision for Go parity.
1461-1521: Beta3 test expectations correctly updated.The "SDL: Basic Beta3" test expectations properly reflect the v3 group structure with the updated field names and nullable patterns.
ts/src/sdl/SDL/parity.spec.ts (2)
85-97: Verify version-agnostic manifest/groups generation is intentional.
validateSchemasalways callssdl.v3Manifest()andsdl.v3Groups()regardless of theversionparameter passed. For v2.0 fixtures usingbeta2, this may not produce the expected v2 format.If parity testing requires v3 output format for both versions, this is fine. Otherwise, consider dispatching based on version:
const manifest = version === "beta2" ? sdl.v2Manifest(false) : sdl.v3Manifest(false); const groups = version === "beta2" ? sdl.v2Groups() : sdl.v3Groups();
123-140: Invalid SDL test block handles missing directory gracefully.The early return pattern inside the
describecallback works but is unconventional. Theit.skipplaceholder ensures the test suite doesn't fail silently when the directory is missing.ts/src/sdl/SDL/SDL.ts (4)
757-764: Price formatting to 18 decimal places.The price amount is formatted with 18 decimal precision to match Go's decimal representation. The logic handles both integer and decimal inputs correctly.
586-599: Improved endpoint sequence computation with Set-based deduplication.The refactored
computeEndpointSequenceNumberscorrectly:
- Uses a
Setfor deduplication- Sorts IPs for deterministic ordering
- Maps to sequence numbers starting at 2
This is cleaner than the previous implementation.
860-875: v2Groups updated with snake_case field naming.The
v2Groupsmethod now usessigned_bywithall_of/any_offield names to match Go output, maintaining consistency with the v3 implementation.
813-817: Asymmetry between v3 and v2 price structures is intentional and schema-aligned.The schema explicitly defines both
price(singular) andprices(array) as valid fields in resource definitions. v3Groups correctly usesprices: [price]to support the v3 schema's array-based pricing model, while v2Groups usesprice: pricefor backward compatibility. This design allows v3 to accommodate multiple prices per resource in the future. No action needed.ts/src/sdl/types.ts (1)
278-285: Snake_case naming aligns with Go output format.The
v3DeploymentGroup.requirementstype correctly updates:
- Snake_case naming (
signed_by,all_of,any_of) matching Go JSON conventions- Nullable arrays (
string[] | null) to represent empty arrays asnullThis is a breaking change to the public type signature. Verify that consumers have been updated to handle the renamed properties.
go/sdl/sdl-input.schema.yaml (5)
1-9: LGTM! Well-structured schema with clear documentation.The schema header correctly uses JSON Schema Draft-07 with
additionalProperties: falseat the root level to enforce strict validation. The description appropriately notes that semantic validations are deferred to the Go parser at runtime.
60-101: Storage class validation logic is correct.The
storageAttributesValidationproperly enforces:
- RAM class cannot have
persistent: true- Non-RAM classes (
beta1,beta2,beta3,default) requirepersistent: trueThe bidirectional boolean handling (accepting both boolean
trueand string"true") is a good defensive measure for YAML parsing variations.
347-375: GPU bidirectional validation is well-implemented.The
allOfconstraints correctly enforce that:
- If
units > 0, thenattributesis required- If
attributesis present, thenunits > 0is requiredThis prevents orphaned GPU attributes without allocation and ensures GPU allocations specify hardware requirements.
479-492: LGTM! Version and required fields are correctly defined.The version enum properly restricts to supported SDL versions ('2.0', '2.1'), and all essential top-level fields are marked as required.
310-376: The schema's limitation to NVIDIA-only GPU vendors is intentional. Test cases ingo/sdl/schema_complex_rules_test.goexplicitly validate that AMD vendors are rejected, and invalid vendor examples are present in test fixtures. No action required.Likely an incorrect or invalid review comment.
| ); | ||
|
|
||
| const sortedIps = Array.from(ips).sort(); | ||
| return Object.fromEntries(sortedIps.map((ip, index) => [ip, index + 2])); | ||
| } |
There was a problem hiding this comment.
Fix trailing whitespace.
Line 597 has trailing whitespace per static analysis.
🔧 Proposed fix
);
-
+
const sortedIps = Array.from(ips).sort();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ); | |
| const sortedIps = Array.from(ips).sort(); | |
| return Object.fromEntries(sortedIps.map((ip, index) => [ip, index + 2])); | |
| } | |
| ); | |
| const sortedIps = Array.from(ips).sort(); | |
| return Object.fromEntries(sortedIps.map((ip, index) => [ip, index + 2])); | |
| } |
🧰 Tools
🪛 GitHub Check: ts
[failure] 597-597:
Trailing spaces not allowed
🤖 Prompt for AI Agents
In @ts/src/sdl/SDL/SDL.ts around lines 596 - 600, The trailing whitespace at the
end of the line that declares "const sortedIps = Array.from(ips).sort();" should
be removed; edit the line containing the "sortedIps" declaration (the line that
uses Array.from(ips).sort()) to eliminate any trailing spaces or tabs so there
is no trailing whitespace before the return that builds
Object.fromEntries(sortedIps.map(...)).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ts/src/sdl/SDL/SDL.spec.ts (1)
546-601: Fix test expectations: Manifest should usesizenotquantityfor memory/storage.The test expectations at lines 546-601 incorrectly use
quantityfield for memory and storage. Both the implementation code (SDL.ts) and all fixture files (manifest.json and groups.json) consistently usesizefield. The expectations should be corrected to:
memory.size.val(notmemory.quantity.val)storage[].size.val(notstorage[].quantity.val)This matches the implementation and aligns with the groups format at lines 618-650.
🧹 Nitpick comments (2)
go/sdl/schema_complex_rules_test.go (1)
698-825: Consider refactoring: table-driven test with hardcoded SDL selection.The test dispatches on
tt.name(line 718) to select the SDL payload, which undermines the table-driven pattern. If test names change or new cases are added, this breaks silently. Consider moving the SDL directly into each test case struct:♻️ Suggested refactor
func TestSchemaValidation_ExposeToService(t *testing.T) { tests := []struct { name string + sdl string shouldErr bool reason string }{ { - name: "valid service routing", - shouldErr: false, + name: "valid service routing", + sdl: `version: "2.0" +services: + web: + image: nginx + expose: + - port: 80 + to: + - service: api + api: + image: node:18 +...`, // full SDL here + shouldErr: false, }, - { - name: "multiple to targets", - shouldErr: false, - }, + // ... additional cases with inline SDL } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - var sdl string - if tt.name == "valid service routing" { - sdl = `...` - } else { - sdl = `...` - } - - err := validateInputAgainstSchema([]byte(sdl)) + err := validateInputAgainstSchema([]byte(tt.sdl))go/sdl/schema_validation_test.go (1)
402-454: Clarify expectation: unquotedversion: 2.0behavior.The test at line 414 expects
version: 2.0(without quotes) to be rejected. This is correct if the schema requires version to be a string and the YAML parser interprets2.0as a float. However, the comment "without quotes" could be clearer about why this fails (type mismatch: float vs string enum).Consider updating the reason field:
{"invalid number", "2.0", true}, // YAML parses as float, schema expects stringThis is a minor documentation improvement.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go/node/deployment/v1beta3/resourceunit.pb.gois excluded by!**/*.pb.gogo/node/gov/v1beta3/params.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
go/node/cert/v1/errors_test.gogo/node/types/deposit/v1/errors_test.gogo/sdl/gpu_test.gogo/sdl/schema_additional_props_test.gogo/sdl/schema_complex_rules_test.gogo/sdl/schema_edge_cases_test.gogo/sdl/schema_validation_test.gogo/tools.gots/src/sdl/SDL/SDL.spec.tsts/src/sdl/SDL/fixtures/gpu_basic_ram_interface.manifest.jsonts/test/fixtures/sdl/groups-basic-snapshot.jsonts/test/fixtures/sdl/manifest-basic-snapshot.json
💤 Files with no reviewable changes (1)
- go/node/cert/v1/errors_test.go
✅ Files skipped from review due to trivial changes (2)
- go/sdl/schema_edge_cases_test.go
- go/node/types/deposit/v1/errors_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go/sdl/gpu_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: go
- GitHub Check: proto
- GitHub Check: test
- GitHub Check: sdl-parity
- GitHub Check: coverage
🔇 Additional comments (14)
go/tools.go (1)
24-24: LGTM!The import reordering is a no-op change. Import order within a single block has no semantic significance in Go, and blank imports will continue to register their side effects as expected.
ts/src/sdl/SDL/fixtures/gpu_basic_ram_interface.manifest.json (1)
82-95: LGTM! Numeric-to-string conversion aligns with parity testing goals.Converting
valfields from numbers to strings ensures consistent JSON serialization between Go and TypeScript parsers. This is a common pattern for handling large integer values and maintaining cross-language compatibility.ts/test/fixtures/sdl/manifest-basic-snapshot.json (1)
49-62: LGTM! Consistent with the updated SDL model.The string-encoded numeric values match the pattern established in other manifest fixtures, ensuring consistent snapshot testing across the codebase.
ts/test/fixtures/sdl/groups-basic-snapshot.json (2)
5-19: LGTM! Structural changes align with Go parser output format.The updates reflect:
signed_bywithall_of/any_ofusing snake_case (matching Go JSON tags)pricesarray with high-precision decimal strings (18 decimal places, standard for cosmos-sdk token amounts)- Nullable fields explicitly set to
nullThese changes ensure the TypeScript snapshot matches the expected Go parser output for parity validation.
23-46: LGTM! Numeric resource values converted to strings consistently.The
valfields for CPU, GPU, memory, and storage are now string-encoded, matching the manifest fixture pattern and ensuring consistent JSON serialization between Go and TypeScript.go/sdl/schema_additional_props_test.go (1)
1-682: LGTM — comprehensive coverage for additionalProperties validation.This test thoroughly covers unknown field rejection across all major SDL structures. The table-driven approach is clean and well-organized.
Minor observation: all test cases have
shouldErr: true, making the else branch (lines 677-679) currently unreachable. This is acceptable for test symmetry and potential future cases, but you could also simplify by removing theshouldErrfield entirely since the test is specifically for rejection scenarios.go/sdl/schema_complex_rules_test.go (1)
1-696: LGTM — comprehensive complex rules validation.The tests for NextCases, SignedBy, GPUAttributes, IPRequiresGlobal, Dependencies, EndpointKind, EnvArgsCommand, and StorageParams are well-structured with good coverage of edge cases and error conditions. The table-driven approach with clear
reasonfields makes the tests self-documenting.Also applies to: 827-936
go/sdl/schema_validation_test.go (1)
1-400: LGTM — thorough schema validation coverage.Excellent coverage across credentials, ports, protocol, denom, CPU units, endpoint names, required fields, and GPU validation. The tests properly cover:
- Boundary values (port 0, 1, 65535, 65536)
- Format variations (uppercase/lowercase protocols, milli units)
- Required vs optional fields
- Complex conditional requirements (GPU units requiring attributes)
The table-driven approach with descriptive reasons makes these tests maintainable.
Also applies to: 456-805
ts/src/sdl/SDL/SDL.spec.ts (6)
25-29: LGTM!The path update from
price.denomtoprices.0.denomcorrectly reflects the new array-based pricing structure in the groups output format.
66-95: LGTM!The endpoint sequence number updates are consistent across both manifest and groups expectations, reflecting the updated sequencing semantics for IP endpoints.
603-659: LGTM!The
expectedGroupsupdates correctly reflect the new field naming conventions (signed_by,all_of,any_of), thequantity→sizemigration for memory/storage, and the addition of the GPU block. The singularpriceobject (vspricesarray) is correct for the beta2 version used in this test.
1296-1352: LGTM!The
expectedGroupsfor the "SDL: Basic" test consistently applies the same format changes:signed_bywith snake_case properties,sizereplacingquantity, GPU block addition, and string-formatted CPU units. Correctly maintains the singularpriceobject for beta2.
1434-1492: LGTM!The beta3 test correctly uses the
pricesarray format with 18-decimal precision amounts ("10000.000000000000000000"), distinguishing it from beta2's singularpriceobject. This aligns with the Go parser output format for v2.1 SDL.
1168-1169: LGTM!The
expectedPreVersionJsonstrings correctly use the updatedsizefield instead ofquantity. These strings are critical for manifest version hash calculation, and the updates ensure consistency with the Go parser output.Also applies to: 1354-1355
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @ts/src/sdl/SDL/SDL.ts:
- Around line 707-729: groupResourceUnits currently type-asserts resource to
v3ComputeResources which is unsafe; change its signature from (resource:
v2ComputeResources | undefined) to (resource: v2ComputeResources |
v3ComputeResources | undefined), remove the cast on the call to resourceUnitGpu
so you call resourceUnitGpu(resource) directly, and ensure callers (v2Groups and
v3Groups) still pass their respective resource types; alternatively, if you
prefer gating GPU, detect presence of a v3-only field (or resource.gpus) and
only call resourceUnitGpu when that field exists, otherwise set units.gpu to the
normalized default—update the signature and callers accordingly to eliminate the
unsafe cast.
- Around line 587-599: The endpoint sequence numbers currently start at 2 due to
using index + 2 in the return mapping inside computeEndpointSequenceNumbers (the
Set/Array building that collects global expose IPs and produces sortedIps);
change the mapping to use index + 1 so sequence numbers start at 1 to match the
Go parser, and update or remove the JSDoc/example near
computeEndpointSequenceNumbers that shows sequence_number: 2 (and adjust any
tests expecting sequence_number: 2 to expect 1).
🧹 Nitpick comments (1)
ts/src/sdl/SDL/SDL.ts (1)
691-705: Consider specifying radix forparseInt.While
parseIntdefaults to base 10 for standard numeric strings, explicitly specifying the radix is a defensive best practice.Suggested fix
- const gpu = isString(units) ? parseInt(units) : units; + const gpu = isString(units) ? parseInt(units, 10) : units;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ts/src/sdl/SDL/SDL.ts
🧰 Additional context used
🧬 Code graph analysis (1)
ts/src/sdl/SDL/SDL.ts (2)
ts/src/sdl/sizes.ts (2)
convertResourceString(12-20)convertCpuResourceString(30-38)ts/src/sdl/types.ts (3)
v3ResourceGPU(197-200)v2ComputeResources(212-216)v3ComputeResources(218-224)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: sdl-parity
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: go
- GitHub Check: proto
🔇 Additional comments (7)
ts/src/sdl/SDL/SDL.ts (7)
168-178: LGTM!The simplification of
resourceUnitandresourceValuemethods improves clarity by removing unnecessary branching. The consistent string conversion aligns with the parity requirements.
247-259: LGTM!The
serviceResourceGpumethod correctly handles optional GPU resources with sensible defaults and consistent structure.
485-495: LGTM!Sorting storage keys ensures deterministic output order, which is essential for parity testing against the Go parser.
602-641: LGTM!The refactored
resourceUnitCpuandresourceUnitMemorymethods improve code clarity with incremental object construction and ensure deterministic output by sorting attributes.
757-764: LGTM!The price amount formatting correctly normalizes values to 18 decimal places for blockchain compatibility, handling both integer and decimal inputs appropriately.
774-789: LGTM!The attribute sorting and
signed_byfield structure with snake_case naming (all_of,any_of) correctly aligns with Go parser output conventions. Converting empty arrays tonullensures parity with Go's JSON serialization.
868-872: LGTM!The v2Groups
signed_byhandling is now consistent with v3Groups, ensuring uniform output structure across both versions.
| ) | ||
|
|
||
| func TestV2ResourceGPU_EmptyVendor(t *testing.T) { | ||
| var stream = ` |
There was a problem hiding this comment.
I changed it to a table tests
| @@ -0,0 +1,102 @@ | |||
| package main | |||
There was a problem hiding this comment.
Maybe we should move these test utility executables into a different folder as cmd should include project specific executables. Perhaps a tools directory? I'm not too opinionated on this though.
There was a problem hiding this comment.
Moved:
- go/sdl/generate-sdl-fixtures/main.go to the go/sdl/tools/generate-sdl-fixtures/main.go.
- testdata/sdl input files to the testdata/sdl/input
- testdata/sdl output files moved to the testdata/sdl/output
There was a problem hiding this comment.
I meant a tools directory at the root of the chain-sdk. The go directory is for the go client-specific code.
There was a problem hiding this comment.
chain-sdk is a multi-language repo, and I would like to keep the clear separation of Go and TS code.
go.work doesn't make root a module (it has go, go/cli and go/sdl).
And generate-sdl-fixtures/main.go uses imports from the sdl package, so I would keep it within that package.
Lets leave the path as go/sdl/tools/generate-sdl-fixtures/main.go.
| @@ -0,0 +1,492 @@ | |||
| $schema: http://json-schema.org/draft-07/schema# | |||
There was a problem hiding this comment.
What's the source of truth here? Can't proto files be used as schema/to generate schema? (Even assuming custom annotations)
There was a problem hiding this comment.
no, it's impossible since there is a difference between SDL input (YAML format) and manifest which is then encoded with protobuf.
There was a problem hiding this comment.
Proto files defines the result after transformation, while this schema defines the user-facing input format.
And I used LLM to generate that schema based on the golang implementation.
Also the schema was partially verified by @stalniy - he highlighted discrepancies and issues.
Now it is more reliable.
Also, in golang SDL part I use this schema not in a strict mode - it doesn't block manifest passing forward in case of the schema validation failure.
Later after observing the logs we can make it more strict, but for the initial schema introducing I think it is enough.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@testdata/sdl/input/invalid/invalid-port.yaml`:
- Line 1: Update the YAML $schema relative path in the test files under
testdata/sdl/input/invalid/ and testdata/sdl/input/schema-only-invalid/ to use
four-levels up instead of three; specifically replace occurrences of
"../../../go/sdl/sdl-input.schema.yaml" with
"../../../../go/sdl/sdl-input.schema.yaml" (look for the line starting with "#
yaml-language-server: $schema=" in files such as invalid-port.yaml and the other
35 files) so the schema resolves to go/sdl/sdl-input.schema.yaml at the
repository root.
In `@testdata/sdl/input/v2.1/credentials/input.yaml`:
- Around line 7-11: The fixture contains realistic-looking credentials under the
credentials block (keys: host, email, username, password); replace them with
unmistakable placeholders to avoid leaking secrets and tripping scanners—e.g.,
set host to example-host or registry.example.com, email to [email protected] (or
placeholder-email), username to example-user, and password to a clear non-secret
like example-password or not-a-secret; update the credentials mapping in the
test fixture so no real usernames, domains, or passwords remain.
♻️ Duplicate comments (1)
testdata/sdl/input/v2.0/ip-endpoint/groups.json (1)
45-48: Add missingkindon the first endpoint entry.Line 47 has only
sequence_number, while all other endpoints includekind. If the schema requireskind, this fixture will be invalid and parity checks may fail. Please align it with the intended endpoint type (e.g.,kind: 1for IP) based on the matching input.🛠️ Suggested fix (confirm the correct kind)
"endpoints": [ { + "kind": 1, "sequence_number": 0 },
🧹 Nitpick comments (7)
testdata/sdl/input/invalid/port-too-high.yaml (1)
8-9: Differentiate this invalid-port case from the other 99999 fixture.This duplicates
invalid-port.yaml. Consider using a distinct boundary case (e.g., 65536 or 0) or remove one to keep coverage focused.✅ Example boundary tweak
- - port: 99999 + - port: 65536testdata/sdl/input/v2.1/credentials/manifest.json (1)
74-75: Add trailing newline for consistency.The file is missing a trailing newline at the end. Most style guides and tools expect files to end with a newline character.
Suggested fix
} ] -] +] +testdata/sdl/input/v2.0/multiple-services/input.yaml (1)
14-33: Consider a less “real-looking” password placeholder.
Line 29 usesPOSTGRES_PASSWORD=secret; for fixtures, a clearer placeholder (e.g.,CHANGE_ME) reduces copy/paste risk and avoids false positives in secret scanners.🧹 Suggested tweak
- - POSTGRES_PASSWORD=secret + - POSTGRES_PASSWORD=CHANGE_MEgo/sdl/tools/generate-sdl-fixtures/main.go (2)
12-14: Hardcoded relative path is fragile.The relative path
"../../testdata/sdl/input"assumes the tool is always run fromgo/sdl/tools/generate-sdl-fixtures/. This could break if invoked from a different directory (e.g., viago runfrom repo root).Consider accepting the fixtures root as a CLI argument or using a well-known environment variable, with the current path as a fallback default.
♻️ Suggested improvement
func main() { - fixturesRoot := "../../testdata/sdl/input" + fixturesRoot := "../../testdata/sdl/input" + if len(os.Args) > 1 { + fixturesRoot = os.Args[1] + } versions := []string{"v2.0", "v2.1"}
64-102: Consider adding trailing newline to generated JSON files.Many editors and tools expect files to end with a newline. The current implementation writes raw JSON without a trailing newline.
♻️ Suggested fix
func generateManifest(obj sdl.SDL, fixtureDir string) error { manifest, err := obj.Manifest() if err != nil { return fmt.Errorf("manifest error: %w", err) } manifestJSON, err := json.MarshalIndent(manifest, "", " ") if err != nil { return fmt.Errorf("JSON marshal error: %w", err) } + manifestJSON = append(manifestJSON, '\n') manifestPath := filepath.Join(fixtureDir, "manifest.json") if err := os.WriteFile(manifestPath, manifestJSON, 0600); err != nil { return fmt.Errorf("write error: %w", err) } fmt.Printf(" Generated %s\n", manifestPath) return nil } func generateGroups(obj sdl.SDL, fixtureDir string) error { groups, err := obj.DeploymentGroups() if err != nil { return fmt.Errorf("groups error: %w", err) } groupsJSON, err := json.MarshalIndent(groups, "", " ") if err != nil { return fmt.Errorf("JSON marshal error: %w", err) } + groupsJSON = append(groupsJSON, '\n') groupsPath := filepath.Join(fixtureDir, "groups.json") if err := os.WriteFile(groupsPath, groupsJSON, 0600); err != nil { return fmt.Errorf("write error: %w", err) } fmt.Printf(" Generated %s\n", groupsPath) return nil }go/sdl/parity_test.go (1)
54-60: Consider usingt.Skipforrequire.FailNowinstead oft.Fatalffor missing fixtures.Using
t.Fatalfhere stops the entire test run if any single fixture is missing. If this is intentional to enforce fixture generation before tests, consider adding a comment explaining this design choice. Otherwise,t.Skipfmight be more appropriate to allow other fixtures to still be tested.♻️ Optional: Use t.Skipf if partial test runs are acceptable
if _, err := os.Stat(manifestPath); os.IsNotExist(err) { - t.Fatalf("manifest.json not generated for %s (run: make generate-sdl-fixtures)", fixtureName) + t.Skipf("manifest.json not generated for %s (run: make generate-sdl-fixtures)", fixtureName) } if _, err := os.Stat(groupsPath); os.IsNotExist(err) { - t.Fatalf("groups.json not generated for %s (run: make generate-sdl-fixtures)", fixtureName) + t.Skipf("groups.json not generated for %s (run: make generate-sdl-fixtures)", fixtureName) }go/sdl/schema_validator.go (1)
133-141: Consider logging the inverse mismatch case (schema fails, Go passes).Currently,
checkSchemaValidationResultonly logs when the schema passes but Go validation fails. For comprehensive mismatch detection, you may also want to log when the schema rejects input but Go accepts it. This would help identify cases where the schema is stricter than the parser.♻️ Proposed enhancement to log both mismatch directions
func (sv *SchemaValidator) checkSchemaValidationResult(schemaErr error, goValidationErr error) { if schemaErr == nil && goValidationErr != nil { logger := sv.logger.Load().(log.Logger) logger.Warn( "SDL schema validation mismatch", "schema_validation", "passed", "go_error", goValidationErr.Error(), ) } + if schemaErr != nil && goValidationErr == nil { + logger := sv.logger.Load().(log.Logger) + logger.Warn( + "SDL schema validation mismatch", + "schema_validation", "failed", + "schema_error", schemaErr.Error(), + "go_validation", "passed", + ) + } }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@go/sdl/parity_test.go`:
- Around line 54-60: Replace the t.Fatalf calls in the manifest and groups
existence checks with non-fatal reporters so one missing fixture doesn't abort
the whole test; specifically, in parity_test.go change the two t.Fatalf calls
(the checks that call os.Stat(manifestPath) and os.Stat(groupsPath)) to either
t.Skipf("... (run: make generate-sdl-fixtures)") if a missing fixture should
skip that case, or t.Errorf("...") if you prefer to record a failure and
continue testing other fixtures—update both checks consistently.
♻️ Duplicate comments (1)
ts/src/sdl/SDL/SDL.ts (1)
807-831: Fix v3Groups aggregation whenlocationis 0 and merge endpoints on the correct field.
Line 809 usesif (!location)which treats index0as “missing”, and Line 829 appends arrays with+=on the wrong object. This breaks aggregation and can corrupt endpoints.🐛 Proposed fix
- if (!location) { + if (location === undefined) { const res = this.groupResourceUnits(compute.resources); res.endpoints = this.v3ServiceResourceEndpoints(service); @@ - } else { - const endpoints = this.v3ServiceResourceEndpoints(service); - // resources.id = group.dgroup.resources[location].id; - - group.dgroup.resources[location].count += svcdepl.count; - group.dgroup.resources[location].endpoints += endpoints as any; - group.dgroup.resources[location].endpoints.sort(); - } + } else { + const endpoints = this.v3ServiceResourceEndpoints(service); + // resources.id = group.dgroup.resources[location].id; + + const resourceEntry = group.dgroup.resources[location]; + resourceEntry.count += svcdepl.count; + const existing = (resourceEntry.resource.endpoints || []) as any[]; + resourceEntry.resource.endpoints = existing.concat(endpoints ?? []); + resourceEntry.resource.endpoints.sort(); + }
🧹 Nitpick comments (4)
go/sdl/sdl.go (1)
92-98: Named returnsdlObjis unused.The named return variable
sdlObjis declared but never assigned. Line 132 returnsobjdirectly. Consider either usingsdlObjconsistently or removing the named return for clarity.♻️ Option 1: Use named return consistently
-func Read(buf []byte) (sdlObj SDL, err error) { +func Read(buf []byte) (SDL, error) { schemaErr := validateInputAgainstSchema(buf) + var err error // Soft check if schema validation passed but the SDL is rejected by the Go parser defer func() { checkSchemaValidationResult(schemaErr, err) }()♻️ Option 2: Assign to sdlObj before returning
- return obj, nil + sdlObj = obj + return sdlObj, nilgo/sdl/tools/generate-sdl-fixtures/main.go (2)
13-14: Hardcoded relative paths require specific working directory.The tool relies on relative paths (
../../testdata/sdl/input), meaning it must be executed from thego/sdl/tools/generate-sdl-fixturesdirectory. Consider accepting paths as command-line flags or environment variables for flexibility, or document the required working directory.♻️ Optional: Accept paths via flags
package main import ( "encoding/json" + "flag" "fmt" "os" "path/filepath" "pkg.akt.dev/go/sdl" ) func main() { - inputRoot := "../../testdata/sdl/input" - outputRoot := "../../testdata/sdl/output-fixtures" + inputRoot := flag.String("input", "../../testdata/sdl/input", "Input fixtures root directory") + outputRoot := flag.String("output", "../../testdata/sdl/output-fixtures", "Output fixtures root directory") + flag.Parse() versions := []string{"v2.0", "v2.1"}
76-84: Consider appending a trailing newline to JSON output.Most text editors and POSIX conventions expect files to end with a newline. This helps avoid "no newline at end of file" warnings in diffs.
♻️ Proposed fix
- if err := os.WriteFile(manifestPath, manifestJSON, 0600); err != nil { + if err := os.WriteFile(manifestPath, append(manifestJSON, '\n'), 0600); err != nil { return fmt.Errorf("write error: %w", err) }Apply the same pattern to
generateGroupsat line 102.go/sdl/parity_test.go (1)
205-220: Consider usingstrings.Joinfor cleaner error formatting.Minor improvement for error message formatting.
♻️ Proposed refactor
+import "strings" + func validateDataAgainstCompiledSchema(data []byte, schema *gojsonschema.Schema) error { result, err := schema.Validate(gojsonschema.NewBytesLoader(data)) if err != nil { return fmt.Errorf("failed to validate against schema: %w", err) } if !result.Valid() { var errors []string for _, desc := range result.Errors() { errors = append(errors, desc.String()) } - return fmt.Errorf("schema validation failed: %v", errors) + return fmt.Errorf("schema validation failed: %s", strings.Join(errors, "; ")) } return nil }
vertex451
force-pushed
the
artem/sdl-parity-tests
branch
from
993c080 to
1d573bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ts/src/sdl/SDL/SDL.ts (1)
824-831: Fix array concatenation: use spread operator orconcatinstead of+=Line 829 uses
+=to concatenate arrays, which coerces them to strings. Theendpointsproperty exists directly on the resource wrapper (not underresource.endpointsas suggested in the proposed fix). Replace with proper array operations:- group.dgroup.resources[location].endpoints += endpoints as any; + group.dgroup.resources[location].endpoints = [ + ...group.dgroup.resources[location].endpoints, + ...endpoints + ];
🤖 Fix all issues with AI agents
In `@go/sdl/schema_validator.go`:
- Around line 23-51: The atomic.Value currently stores different concrete logger
implementations (panic risk); change the stored type to a single concrete
wrapper so all Store calls use the same concrete type: introduce a small
concrete type (e.g., type loggerHolder struct { L log.Logger }) and update
init() to store loggerHolder{L: noop.NewLogger()} and SetSchemaLogger to store
loggerHolder{L: logger}, and update any Load() usages to cast to loggerHolder
and access the .L field (references: SchemaValidator.logger, defaultValidator,
init(), SetSchemaLogger).
In `@go/sdl/tools/generate-sdl-fixtures/main.go`:
- Around line 12-68: The hardcoded "../../..." inputRoot/outputRoot in main
causes paths to be resolved from the process CWD and skips fixtures when run
from the repo root; change initialization of inputRoot and outputRoot to be
repo-root-relative by first locating the repository root (for example by walking
up from the current working directory until a sentinel file like "go.mod" is
found or using runtime.Caller to locate the source tree) and then set inputRoot
:= filepath.Join(repoRoot, "testdata", "sdl", "input") and outputRoot :=
filepath.Join(repoRoot, "testdata", "sdl", "output-fixtures"); update main to
compute repoRoot before the loop so subsequent logic (the code that reads
inputPath and calls generateManifest and generateGroups) uses correct absolute
paths.
🧹 Nitpick comments (2)
.gitignore (1)
28-31: LGTM! Coverage files appropriately ignored.The new entries correctly prevent coverage output files from being committed, which aligns with the PR's addition of test coverage tooling for different components.
Optional: Consider using a glob pattern for maintainability
If additional coverage files are expected in the future, you could replace these three entries with a single pattern:
-# coverage output files -coverage-go-cli.txt -coverage-go-sdl.txt -coverage-go.txt +# coverage output files +coverage-go*.txtHowever, explicit entries are perfectly fine if these are the only expected coverage files.
specs/sdl/README.md (1)
22-22: Consider future-proofing GPU vendor documentation.The documentation states "GPU vendor (nvidia only)" as a strict rule. If additional GPU vendors (e.g., AMD, Intel) are supported in the future, this documentation will need updating. Consider phrasing it more generically or adding a note about checking the schema for the current list of supported vendors.
vertex451
force-pushed
the
artem/sdl-parity-tests
branch
from
1d573bb to
04de1c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@go/sdl/tools/generate-sdl-fixtures/main.go`:
- Around line 35-39: The code currently checks for input.yaml with
os.Stat(inputPath) and silently continues when missing; change this to fail fast
so missing fixtures are not silently skipped: replace the continue in the
os.IsNotExist(err) branch with a failing action (e.g., log.Fatalf or return an
error) that includes the inputPath and context, so callers of main.go (or the
enclosing function) clearly see which fixture directory is missing input.yaml;
keep the rest of the os.Stat error handling intact.
In `@specs/sdl/groups-output.schema.yaml`:
- Around line 51-83: Update the schema so the singular price object matches
DecCoin and fixtures by requiring both amount and denom: in the price property
(the object with properties amount and denom) add denom to the required list so
required: [ "amount", "denom" ] for the price definition; for the prices
(plural) definition decide whether it’s dead code—either remove the prices
property entirely if it’s unused in fixtures/generated output, or add a
TODO/comment and implement/align its usage if it represents a planned feature,
ensuring its item schema also requires both amount and denom consistently with
DecCoin.
🧹 Nitpick comments (4)
.github/workflows/tests.yaml (1)
57-76: Use env.NODE_VERSION instead of hardcoding 22.14.0 in sdl-parity.The Node.js version is centrally defined in
.envrcasNODE_VERSION=$(jq -r '.volta.node' ts/package.json)and is already used by thecoverageandlintworkflows. Using the hardcoded22.14.0on line 70 creates version drift and inconsistency.♻️ Suggested change
- node-version: 22.14.0 + node-version: "${{ env.NODE_VERSION }}"testdata/sdl/output-fixtures/v2.0/simple/groups.json (1)
1-67: LGTM! Valid groups fixture for v2.0 parity testing.The fixture structure is well-formed with proper string representations for resource values, consistent with the parity testing approach. The
signed_bystructure withall_of/any_ofarrays and the dual endpoint configuration provide good coverage.Consider adding a trailing newline at the end of the file for POSIX compliance and to avoid "No newline at end of file" warnings in diffs.
specs/sdl/manifest-output.schema.yaml (1)
23-24: Tightencountto integer for stricter validation.
countis a discrete replica value; usingintegerprevents fractional outputs (and you can apply the same to other count/port/id fields if desired).♻️ Suggested tweak
- count: - type: number + count: + type: integergo/sdl/schema_edge_cases_test.go (1)
126-175: Consider adding negative test cases for hostname validation.The current tests only verify that valid hostnames are accepted. Consider adding cases for invalid hostnames to ensure the schema properly rejects them:
- IP addresses (e.g.,
192.168.1.1)- Invalid characters (e.g.,
host_name.comwith underscore in domain)- Empty strings
- Hostnames exceeding length limits
♻️ Example additional test cases
{ name: "wildcard_subdomain", builder: sdlTestBuilder{exposeBlock: ` expose: - port: 80 accept: - "*.example.com" to: - global: true`}, shouldErr: false, }, + { + name: "invalid_ip_address", + builder: sdlTestBuilder{exposeBlock: ` expose: + - port: 80 + accept: + - 192.168.1.1 + to: + - global: true`}, + shouldErr: true, + reason: "IP addresses should not be accepted as hostnames", + }, }
Parity Testing
./testdata/sdl/contains test scenarios. Each directory has:input.yaml— SDL inputmanifest.json— Generated manifest (from Go parser)groups.json— Generated deployment groups (from Go parser)Tests validate both parsers(Golang and TS) produce identical output from the same input.
Resolves
akash-network/support#408
Schema Validation
Three schemas validate the SDL pipeline:
sdl-input.schema.yaml— Validates input YAML (required fields, formats, enums, patterns)manifest-output.schema.yaml— Validates manifest JSON outputgroups-output.schema.yaml— Validates groups JSON outputTest Fixtures
testdata/sdl/v2.0/,v2.1/— Valid fixtures for parity teststestdata/sdl/invalid/— Both schema and parsers rejecttestdata/sdl/schema-only-invalid/— Schema rejects, Go accepts (stricter schema rules)IDE validation
IDE supports YAML validation against the schema when creating manifests in the editor.
Important note:
🔧 Purpose of the Change
📌 Related Issues
✅ Checklist