Skip to content

place all definitions at the same level #84

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

rogpeppe
Copy link
Contributor

The JSON Schema draft-06 says of the definitions keyword:

This keyword's value MUST be an object. Each member value of this object MUST be a valid JSON Schema.

In the buildkite schema, the definitions/commonOptions property is used
to namespace schemas, not as a schema itself, which doesn't seem
to fit the intended structure for definitions, even though technically it's just about
OK because none of the properties happen to coincide with keywords
defined in draft-06 ("if" was only defined in draft-07) and unknown keywords
are ignored.

Referring to definitions that aren't in regular schema locations is also
in a grey area. Later versions of the spec have this to say:

a reference target under a known keyword, for which the value is known not to be a schema, results in undefined behavior

Even though draft-06 doesn't include this wording, it seems that the reasoning
still applies.

So to avoid the above issues, this PR moves all the definitions that
were inside commonOptions into the top level definitions property.

The distinction between the two kinds of definitions is still clear by virtue
of the Step suffix used for all the actual step definitions and not for
the others.

Some other possibilities:

  1. give all the schemas inside commonOptions a common prefix, such as "common.", so we'd have #/definitions/common.env etc.
  2. move all the definitions inside commonOptions one level down into another definitions property, so we'd have #/definitions/commonOptions/definitions/env etc.

Both of those seem unneccessarily verbose to me, but YMMV.

I have verified that the tests pass with this change in place,
assuming #83 is landed as a prereq.

I've split this PR into two commits to make the diffs easier to see.
The first one leaves the indentation inplace; the second aligns it correctly.

The JSON Schema draft-06 [says](https://json-schema.org/draft-06/draft-wright-json-schema-validation-01#rfc.section.7.1) of the `definitions` keyword:

> This keyword's value MUST be an object. Each member value of this object MUST be a valid JSON Schema.

In the buildkite schema, the `definitions/commonOptions` property is used
to namespace schemas, not as a schema itself, which doesn't seem
to fit the intended structure for `definitions`, even though technically it's just about
OK because none of the properties happen to coincide with keywords
defined in draft-06 (`"if"` was only defined in draft-07) and unknown keywords
are ignored.

Referring to definitions that aren't in regular schema locations is also
in a grey area. Later versions of the spec have [this to say](https://json-schema.org/draft/2020-12/draft-bhutton-json-schema-01#section-9.4.2):

> a reference target under a known keyword, for which the value is known not to be a schema, results in undefined behavior

Even though draft-06 doesn't include this wording, it seems that the reasoning
still applies.

So to avoid the above issues, this PR moves all the definitions that
were inside `commonOptions` into the top level `definitions` property.
We leave the indentation as it was to make the diff clearer,
leaving the indentation fixup for a later commit.

The distinction between the two kinds of definitions is still clear by virtue
of the `Step` suffix used for all the actual step definitions and not for
the others.

Some other possibilities:

1. give all the schemas inside `commonOptions` a common prefix, such as "common.", so we'd have `#/definitions/common.env` etc.
2. move all the definitions inside `commonOptions` one level down into another `definitions` property, so we'd have `#/definitions/commonOptions/definitions/env` etc.

Both of those seem unneccessarily verbose to me, but YMMV.

I have verified that the tests pass with this change in place,
assuming buildkite#83 is landed as a prereq.

Signed-off-by: Roger Peppe <[email protected]>
The previous commit moved a bunch of schemas from
commonOptions to one level up. We left the indentation
in place to make it easier to see what changes had been
made. This PR just removes two spaces from each line
that was inside commonOptions.

Signed-off-by: Roger Peppe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants