Server Native YAML Integration.#96
Conversation
…r yaml config format introduced in asd 8.1.1.0 server-497
|
Strange, tests are failing on |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #96 +/- ##
==========================================
+ Coverage 81.05% 81.75% +0.69%
==========================================
Files 15 22 +7
Lines 2101 3113 +1012
==========================================
+ Hits 1703 2545 +842
- Misses 296 412 +116
- Partials 102 156 +54 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ver native yaml code to its own package
# Conflicts: # testdata/cases/server811/conf-tests.json # testdata/cases/server811/yaml-tests.json
dwelch-spike
changed the title
|
Much of this will be removed from asconfig when the management lib supports the new format, hopefully the server-yaml package can be reused for some of that. I think it's important that asconfig have support ready early though. |
There was a problem hiding this comment.
Pull request overview
Adds experimental, first-class support for Aerospike server-native YAML (8.1.0+) across asconfig commands by introducing a dedicated conf/serveryaml package, embedding/using the experimental JSON schemas for validation, and updating integration/unit tests + fixtures to cover round-trips and asd --experimental execution.
Changes:
- Introduce
conf/serveryamlfor native-YAML version gating, schema resolution/validation, and legacy<->native translation. - Embed experimental schemas via
schema.NewExperimentalSchemaMap()and add unit tests to ensure both legacy and experimental schema embedding work. - Extend test harness + fixtures (server 8.1.1/8.1.2) to run experimental YAML cases and validate
--server-yamlflows.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| testutils/utils.go | Adds ServerExperimental flag to test case struct for experimental server runs. |
| testdata/cases/server812/yaml-tests.json | Adds server-native YAML convert fixture test with ServerExperimental. |
| testdata/cases/server812/server812.experimental.yaml | Adds server-native YAML fixture for 8.1.2. |
| testdata/cases/server812/conf-tests.json | Adds conf->native-yaml test for 8.1.2. |
| testdata/cases/server811/yaml-tests.json | Adds server-native YAML convert fixture test with ServerExperimental. |
| testdata/cases/server811/server811.experimental.yaml | Adds server-native YAML fixture for 8.1.1. |
| testdata/cases/server811/conf-tests.json | Adds conf->native-yaml test for 8.1.1. |
| schema/schemamap.go | Adds embedded experimental schema FS and NewExperimentalSchemaMap. |
| schema/schemamap_test.go | Unit tests for experimental schema embedding + legacy/experimental separation. |
| integration_test.go | Runs asd --experimental for flagged fixtures; wires --server-yaml into diff/convert-back paths. |
| conf/serveryaml/format.go | Defines min supported version + gating helpers for server-native YAML. |
| conf/serveryaml/schemas.go | Loads/chooses best experimental schema per version; sanitizes unsupported regex patterns. |
| conf/serveryaml/schemas_test.go | Unit tests for schema loading, resolution, sanitization, and error cases. |
| conf/serveryaml/validate.go | Validates native YAML against experimental JSON schema via gojsonschema. |
| conf/serveryaml/translate.go | Translates server-native YAML into legacy asconfig YAML shape (incl. units + logging). |
| conf/serveryaml/units.go | Converts {value, unit} objects to scalar integers with overflow checks. |
| conf/serveryaml/emit.go | Translates legacy YAML into server-native YAML shape (incl. logging normalization). |
| conf/serveryaml/serveryaml_test.go | Unit tests for translation, version gating, and schema validation behavior. |
| conf/serveryaml/translate_test.go | Unit tests for structural translation edge cases and determinism helpers. |
| conf/serveryaml/units_test.go | Unit tests for unit conversion behavior, overflow, and numeric decoding. |
| conf/serveryaml/emit_test.go | Unit tests for native emission and logging shape normalization. |
| cmd/server_yaml.go | Adds --server-yaml helpers for parse/emit/diff translation + schema error formatting. |
| cmd/server_yaml_test.go | Unit tests for --server-yaml helper behaviors and end-to-end convert wiring. |
| cmd/validate.go | Adds --server-yaml support to validate flow via prepareYAMLForParse. |
| cmd/validate_test.go | Adds unit tests for server-native YAML validate success, version gating, schema rejection. |
| cmd/convert.go | Adds --server-yaml parse/emit hooks and metadata handling for convert. |
| cmd/convert_test.go | Unit tests for convert --server-yaml gating and no-op behavior on non-YAML side. |
| cmd/diff.go | Adds --server-yaml to diff commands and native->legacy translation for diff files; validate+translate for diff server. |
| cmd/generate.go | Adds --server-yaml output emission hook + gating for YAML generation. |
| cmd/generate_test.go | Unit tests for generate --server-yaml gating and non-YAML no-op. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@a-spiker Let me know what you think. This is a lot of changes for server native yaml but they are mostly contained to their own package. Hopefully we can move that code to the management lib eventually and then remove most of it. |
|
PR Review (version 2026.05.27) Promises:
7 findings posted inline (0 Critical, 1 Important, 6 Minor). The one Important is a real unit-conversion bug: Note on the version gate, overflow checks, validation-skip symmetry, and |
| "interval", | ||
| "duration", | ||
| "sleep", | ||
| "age", |
There was a problem hiding this comment.
From Logic agent: Important: isDurationPath uses strings.Contains(field, token) over a token list that includes "age". The namespace byte-size fields index-stage-size and sindex-stage-size contain the substring "stage", so they are misclassified as duration fields. For a m/M unit, getUnitMultiplier then returns unitMinute (60) instead of unitMega (1,000,000).
This is reachable through validated native input: the 8.1.1 schema declares index-stage-size with oneOf allowing {value: 135..17179, unit: "M"} (megabytes). {value: 256, unit: M} translates to the legacy scalar 15360 instead of 268435456 (off by ~16,667x), and 15360 is far below the field’s own integer-form minimum of 134217728, so the resulting legacy config is silently invalid. Verified: getUnitMultiplier("m", [...,"index-stage-size"]) returns 60; the control field data-size correctly returns 1000000.
Fix: match whole --delimited segments instead of substrings (e.g. split the field name and compare tokens, or check suffix tokens), or special-case the *-stage-size family. The other duration tokens (*-period, *-ttl, *-interval, *-duration, tomb-raider-eligible-age, etc.) classify correctly; only the two stage-size fields are wrong.
| } | ||
|
|
||
| for key, value := range typedNode { | ||
| translated, err := translateUnitValues(value, append(path, key)) |
There was a problem hiding this comment.
From Data Flow agent: Minor: The map branch calls translateUnitValues(value, append(path, key)), while the sibling slice branch (line 50) deliberately uses pathWithIndex(path, i), which allocates a fresh slice so recursion does not stomp the caller’s path backing array. With append, successive sibling iterations can reuse the same backing array and overwrite a slot a prior call’s path still references, so a formatNodePath error message can name the wrong field. The conversion result is unaffected (paths feed only error strings), so this is diagnostics-only, but the inconsistency makes a reader wonder whether the difference is intentional. It is not. Use the same copy discipline in both branches (mirror pathWithIndex, or append(append([]string{}, path...), key)).
| return keys | ||
| } | ||
|
|
||
| func sortStrings(keys []string) { |
There was a problem hiding this comment.
From Style agent: Minor: sortStrings hand-rolls an insertion sort to sort collection keys. The sort package is already imported in this package (schemas.go uses sort.Slice), so sort.Strings(keys) does exactly this in one line, is the idiom every Go reader expects, and avoids making the reader verify the loop bounds and swap logic. Replace the sortStrings(keys) call (line 348) with sort.Strings(keys) and delete this function. Matters more given this package is meant to be lifted into aerospike-management-lib.
| // Collect top-level logging context keys (e.g. `any: info`) and merge them | ||
| // into the contexts map. If the same context appears under both the | ||
| // explicit `contexts` map and as a top-level field we error, mirroring | ||
| // the translate.go direction so the round-trip is symmetric. |
There was a problem hiding this comment.
From Style agent: Minor: This comment claims the conflict handling mirrors translate.go "so the round-trip is symmetric," but the two are not mirror images. collectLegacyLoggingContexts errors only when the same context key appears both under contexts and as a top-level field with a different value, whereas flattenServerLoggingContexts (translate.go) errors when a context name collides with any existing sink field regardless of value. The round-trip does not actually require them to match, so the comment sends the reader chasing a symmetry that is not there. Tighten the wording to describe what this function actually guards against, or drop the "mirroring / symmetric" claim.
| replacementPattern = `^[a-zA-Z0-9_\\-$]+$` | ||
| ) | ||
|
|
||
| return strings.ReplaceAll(body, negativeLookaheadPattern, replacementPattern) |
There was a problem hiding this comment.
From Security agent: Minor: sanitizeExperimentalSchema does a single literal strings.ReplaceAll for exactly one known-incompatible pattern. Today that is correct and it fails closed (an unsanitized lookahead makes gojsonschema fail to compile, which surfaces as a hard validation error rather than a silent skip). The risk is the reverse: when a future submodule bump introduces a new gojsonschema-incompatible construct, this substitution will not catch it and every --server-yaml invocation for that version becomes a hard error until someone updates this literal. Add a guard test that loads every embedded experimental schema, runs it through the sanitizer, and asserts it compiles under gojsonschema. That turns a future silent breakage into a CI failure at submodule-bump time.
| // Package serveryaml provides translation, schema validation, and version | ||
| // gating for the Aerospike server-native (experimental) YAML format. | ||
| // | ||
| // The package is self-contained with no dependencies on asconfig/cmd or cobra |
There was a problem hiding this comment.
From Architecture agent: Minor: This doc says the package has "no dependencies on asconfig/cmd or cobra," which is literally true but understates the real lift blocker. schemas.go imports github.com/aerospike/asconfig/schema for SchemaMap / NewExperimentalSchemaMap and the embedded experimental schema FS, so lifting this package into aerospike-management-lib also requires moving (a) the embedded schema files and (b) the NewExperimentalSchemaMap/collector plumbing. The schema package is a clean leaf, so the rework is bounded, but "self-contained" + "minimal rework" should name that one external dependency. Either move the experimental-schema embed + a small loader into conf/serveryaml (true self-containment) or amend the comment to state the asconfig/schema dependency that must accompany the lift.
| return err | ||
| } | ||
|
|
||
| nativeValidated, err := serverYAMLValidatesInput(cmd, srcFormat) |
There was a problem hiding this comment.
From Tests agent: Minor: The convert path has a dedicated regression test that a native-only field is accepted with --server-yaml and no --force (TestConvertServerYAMLAcceptsNativeOnlyFields), proving the legacy validator is skipped. The validate path does not: TestRunEValidateServerYAMLCompat uses a fixture containing only fields that also exist in the legacy schema, so it would pass whether or not this nativeValidated skip branch fires. A regression that re-enabled the legacy validator on the validate path would not be caught. Add a validate-side test using a native-only field (and ideally a sibling assertion that the same file is rejected without --server-yaml). Same gap on the diff live/server path, where prepareYAMLForParse validates the local file (diff.go) but has no --server-yaml test exercising it.
Summary
Adds first-class support in
asconfigfor the server-native (experimental) YAML format introduced in Aerospike 8.1.0. A single context-sensitive--server-yamlflag toggles the format for whichever side of the command is YAML, validation runs against the new experimental schemas before any translation occurs, and the legacy asconfig YAML shape remains the default so existing workflows are unchanged.All native-format logic lives in a new
conf/serveryamlpackage so it can be lifted intoaerospike-management-libonce the library adopts the native shape.Motivation
Previously the new format was exposed through two clunky flags (
--server-yamland--server-yaml-output), and all "validation" happened against the legacy schema after translation — so the experimental schemas weren't actually enforced and, worse, native-only fields would be rejected by the legacy validator forcing users to reach for--force. We want:schema/schemas/json/aerospike-server/*.json).aerospike-management-libwith minimal rework.What changed
One flag, context-sensitive
--server-yamlis now a single flag available onvalidate,convert,diff files,diff server, andgenerate. It applies to whichever side of the operation is YAML:validate,convert yaml -> conf,diff *)convert conf -> yaml,generate -F yaml)type+contexts,{value, unit}sizes, etc.).convert --server-yaml conf -> yamlandconvert --server-yaml yaml -> confboth work with a single flag.The old
--server-yaml-outputflag has been removed.Version gating
validate,convert,diff server, andgeneraterequireaerospike-server-version >= 8.1.0when--server-yamlis set. Patch versions are ignored, so 8.1.0, 8.1.1, 8.1.2, ... all qualify. Missing or below-cutoff versions produce a hard error instead of silently falling back.diff serverfetches the cluster version first, then applies the gate to the local file.diff filesis version-agnostic and does no schema validation.--server-yamlonly signals that the input is in the native shape soasconfigcan translate it for the diff.New
conf/serveryamlpackageAll native-format logic is consolidated into one self-contained package, ready to be lifted into
aerospike-management-lib:format.go—MinSupportedVersion = "8.1.0"andIsSupportedVersion.schemas.go— embeds the experimental schemas, resolves the best schema for a given version (exact match -> highest version <= target -> lowest in the same minor), and sanitizesgojsonschema-incompatible regex patterns at load time.validate.go—Validate(yamlBytes, version)against the experimental schema, returningValidationErrorvalues formatted consistently with existingconf.ValidationErrors.translate.go—ToLegacy: map-keyed collections -> named slices,{value, unit}-> scalar ints, logging normalization.emit.go—FromLegacy: the inverse transform.units.go— unit conversion with overflow checks.The old
cmd/server_yaml_compat.goand its test file were removed; that logic now lives inconf/serveryamland is reached through thin helpers incmd/server_yaml.go(prepareYAMLForParse,translateNativeYAMLForDiff,maybeEmitNativeYAML).Integration coverage
testdata/cases/server811/server811.experimental.yamlandtestdata/cases/server812/server812.experimental.yaml.conf-tests.jsonandyaml-tests.jsonentries for both 8.1.1 and 8.1.2 that round-trip through--server-yaml(conf -> native yaml -> conf) and diff against the hand-written conf source.serverExperimentalfield onTestDataso the harness launchesasd --experimentalfor flagged cases; that same flag also forces the YAML-side diff and the convert-back step to use--server-yaml.schema/schemassubmodule bumped to pick up the 8.1.2 native-YAML schema alongside the existing 8.1.1 native schema.Behavior matrix
--server-yamloff--server-yamlonvalidate ... .yamlvalidate ... .confconvert .conf -> .yamlconvert .yaml -> .confdiff files a bdiff server filefileif it's YAML, gated on the live cluster's version.generate -F yamlgenerate -F confQuick rules
--server-yaml.--server-yaml.asdwith--experimental.Example
Convert
testconf.confto server-native YAML, then run Aerospike against that YAML file:Where
testconf.confis:And
testconf.yamlis:Test plan
go build ./... && go vet ./...go test -tags=unit ./cmd/... ./conf/serveryaml/...(covers new 8.1.2 schema resolution case).server811.experimental.yamlfixture.server812.experimental.yamlfixture (bothconf-testsandyaml-testsexercise--server-yaml).asconfig convert --server-yaml -a 8.1.2.0 testconf.confemits native-shape YAML.asconfig convert --server-yaml -a 8.1.2.0 testconf.yaml -o out.confround-trips back to legacy.conf.asconfig validate --server-yaml -a 8.0.0 file.yamlerrors with the version-gate message.asconfig diff files --server-yaml a.yaml b.yamldiffs without schema validation.Follow-ups
conf/serveryamlintoaerospike-management-libonce the library can consume the native shape directly; at that point the translation layer incmdcollapses to a thin passthrough.--legacy-yaml) rather than opt-in.