Custom annotations for bundle-specific JSON schema fields #1957

ilyakuz-db · 2024-12-04T22:02:09Z

Changes

Adds annotations to json-schema for fields which are not covered by OpenAPI spec.

Custom descriptions were copy-pasted from documentation PR which is still WIP so descriptions for some fields are missing

Further improvements:

documentation autogen based on json-schema
fix missing descriptions

Tests

This script is not part of CLI package so I didn't test all corner cases. Few high-level tests were added to be sure that schema annotations is in sync with actual config

bundle/internal/schema/annotations.yml

libs/jsonschema/extension.go

ilyakuz-db · 2024-12-04T22:07:24Z

libs/jsonschema/schema.go

+
+	// Title of the object, rendered as inline documentation in the IDE.
+	// https://json-schema.org/understanding-json-schema/reference/annotations
+	Title string `json:"title,omitempty"`


Title is not necessary but it's in the spec and monaco editor (vscode, Workspace) supports it. Maybe it will be useful in some cases where the field name itself is ambiguous

How does the title render in the inline IDE interpolation? Is there an improvement vs no title?

Answered here #1957 (comment)

shreyas-goenka

Thanks! This functionality would be great to have. I have a couple of differing opinions about how broad the scope of custom annotations should be and how to structure them.

Please TAL!

Makefile

shreyas-goenka · 2024-12-09T09:21:03Z

bundle/internal/schema/main.go

+func generateSchema(workdir, outputFile string) {
+	annotationsPath := path.Join(workdir, "annotations.yml")
+	annotationsOpenApiPath := path.Join(workdir, "annotations_openapi.yml")
+	annotationsOpenApiOverridesPath := path.Join(workdir, "annotations_openapi_overrides.yml")


I think we should limit the scope of custom annotations to only non-OpenAPI overrides. I don't see much if any overrides for OpenAPI fields. Any such one-of overrides can be easily be added similar to removeJobsFields.

My reasoning is that annotations_openapi.yml and annotations_openapi_overrides.yml are large files that serve little purpose and complicate understanding how OpenAPI descriptions are loaded onto the schema.

The idea was to use annotations_openapi.yml as untouched auto-generated file (we don't modify it), to be able to generate schema without running genkit every time. And in annotations_openapi_overrides.yml we can add any changes we need, e.g. fill missing descriptions.

Currently annotations_openapi_overrides consists only the fields where descriptions are missing, and it seems valid to override in this case since it affects json-schema usability

But we could merge all overrides in single file together in annotations.yml, @pietern WDYT?

We should be cautious about overexposing here since this will be harder to maintain as we launch SDK v1 with versioning per package. The interface of PkgName + TypeName might not be the best one for structs outside the CLI repo.

It seems valid to override in this case since it affects json-schema usability

Yeah, but we should not maintain docs for multiple API fields that we do not own. One-off overrides can be achieved via removeJobsFields.

Could you elaborate more about SDK v1, how it will change json-schema, will it also include different versions?

I'd like to keep it around but as minimal as possible. Good to have an escape hatch.

Could you elaborate more about SDK v1, how it will change json-schema, will it also include different versions?

IIRC we'll have different Go packages for each version, jobsv1, jobsv2 etc so the package name in the key might no longer correspond to the new package names.

We embed these types and we infer their path from the actual struct.

The only issue here is that we add another place where we depend on these paths and that we need to update when this move happens. That's a reasonable cost and worth it, IMO.

I'd like to keep it around but as minimal as possible. Good to have an escape hatch.

We can skip descriptions for those and I will remove all PLACEHOLDER references in OpenAPI overrides and keep the file empty until we need overrides. As for annotations_openapi.yml - we could keep as is to keep an ability to run schema updates without genkit, @shreyas-goenka WDYT?

The only issue here is that we add another place where we depend on these paths and that we need to update when this move happens. That's a reasonable cost and worth it, IMO.

I will add test that checks that descriptions are still valid after path changes as suggested here

bundle/internal/schema/main_test.go

shreyas-goenka · 2024-12-09T09:28:19Z

bundle/internal/schema/annotations.yml

+github.com/databricks/cli/bundle/config.Artifact:
+  description: Defines the attributes to build an artifact.
+  title: Artifact
+github.com/databricks/cli/bundle/config.Artifact.build:


What do you think about grouping the struct fields together? Something like:

github.com/databricks/cli/bundle/config.Artifact: executable: description: PLACEHOLDER files: description: PLACEHOLDER path: description: PLACEHOLDER type: description: PLACEHOLDER

It'll make this file easier to read and maintain. We don't need any docs for the top level Root{} struct so we are not missing out on completeness.

Good idea! Looks like it should be safe to skip non-property descriptions like github.com/databricks/cli/bundle/config.Artifact since it is already included in config.Root.artifacts as property

Updated according to this idea, for some rare cases where we still need root type I added "_" key

shreyas-goenka · 2024-12-09T09:29:38Z

libs/jsonschema/schema.go

+
+	// Title of the object, rendered as inline documentation in the IDE.
+	// https://json-schema.org/understanding-json-schema/reference/annotations
+	Title string `json:"title,omitempty"`


How does the title render in the inline IDE interpolation? Is there an improvement vs no title?

shreyas-goenka · 2024-12-09T09:35:58Z

bundle/internal/schema/annotations.go

+}
+
+func (d *annotationHandler) addAnnotations(typ reflect.Type, s jsonschema.Schema) jsonschema.Schema {
+	refPath := jsonschema.TypePath(typ)


I'd rather keep typePath a private method because it deals with how to serialize #ref and work with the way our OpenAPI spec is structured.

We can instead directly use typ.PkgPath() + "." + typ.Name() here and error if either of them is an empty string. The rationale being that the annotations override will only be applicable for Go structs and not other types like primitive strings.

ilyakuz-db · 2024-12-09T10:23:46Z

How does the title render in the inline IDE interpolation? Is there an improvement vs no title?

It is shown as header, I think the only improvement that we can add some custom title that we want to emphasize, e.g. for target keys, showing something like <target>. Although I'm not sure if schema supports descriptions for non-static keys, need to be checked

shreyas-goenka · 2024-12-09T10:27:06Z

bundle/internal/schema/annotations.yml

Can we add a test to prevent package renames from breaking documentation? A test that ensures all types defined in the annotations overrides actually exist in the CLI repo.

We should also consider limiting the scope of the annotations to only structs in the CLI repo if the keys are based of the type.

Good idea, will add tests for that

We should also consider limiting the scope of the annotations to only structs in the CLI repo if the keys are based of the type.

Do you mean to check properties only for structs that are in the CLI package?

Added test for detached annotations

Makefile

pietern · 2024-12-09T11:01:41Z

bundle/internal/schema/annotations.go

+	"reflect"
+	"strings"
+
+	"github.com/ghodss/yaml"


You can use yamlsaver for saving to disk.

This gives you some control over the node style if you need it (e.g. use blocks for all descriptions).

Updated to yamlsaver with yaml.LiteralStyle to make editing easier, this style seems the best for this purpose

pietern · 2024-12-09T11:20:12Z

bundle/internal/schema/main.go

+func generateSchema(workdir, outputFile string) {
+	annotationsPath := path.Join(workdir, "annotations.yml")
+	annotationsOpenApiPath := path.Join(workdir, "annotations_openapi.yml")
+	annotationsOpenApiOverridesPath := path.Join(workdir, "annotations_openapi_overrides.yml")


I'd like to keep it around but as minimal as possible. Good to have an escape hatch.

….typePath` function

pietern · 2024-12-17T10:44:50Z

Makefile

+schema:
+	@echo "✓ Generating json-schema ..."
+	@go run ./bundle/internal/schema ./bundle/internal/schema ./bundle/schema/jsonschema.json


Why does this need the path to itself?

I passed this as argument to avoid hardcoding relative paths in Go (to keep an ability to execute both from make schema and from other places like Run from IDE). Other possible option to keep portability is to use embed but I also need write operations so it seems like not an option in my case

bundle/internal/schema/annotations.go

pietern · 2024-12-17T10:50:18Z

bundle/internal/schema/annotations.go

+
+	rootTypeAnnotation, ok := annotations[RootTypeKey]
+	if ok {
+		assingAnnotation(&s, rootTypeAnnotation)


Typo

Suggested change

assingAnnotation(&s, rootTypeAnnotation)

assignAnnotation(&s, rootTypeAnnotation)

pietern · 2024-12-17T10:52:09Z

bundle/internal/schema/annotations.go

+		item, ok := annotations[k]
+		if !ok {
+			item = annotation{}
+		}


The value of the map is a value type, so item will already be zero-initialized if it doesn't exist.

The if !ok is not needed.

bundle/internal/schema/annotations.go

bundle/internal/schema/annotations_openapi_overrides.yml

pietern · 2024-12-17T11:08:03Z

bundle/internal/schema/main.go

 		addInterpolationPatterns,
 	})
 	if err != nil {
 		log.Fatal(err)
 	}

+	err = a.sync(annotationsPath)


Can you include a comment here; I suspect this writes the input file back with missing types included.

bundle/internal/schema/main_test.go

pietern · 2024-12-17T11:12:08Z

bundle/internal/schema/main_test.go

+	assert.NoError(t, err)
+	copied, err := os.ReadFile(annotationsPath)
+	assert.NoError(t, err)
+	assert.Equal(t, string(original), string(copied), "Missing JSON-schema descriptions for new config fields in bundle/internal/schema/annotations.yml")


This doesn't run on Windows because it uses path.Join instead of filepath.Join. If you search/replace that the test will work on Windows as well. Not a blocking change for this PR, but would rather not have Windows-skips if it is not strictly necessary.

There was also an issue with line-breaks so I changed the way how this test compares values, now it parses file content and compare dynamic representation of yaml fields

…` method

…file content

…otations-json-schema

pietern

Thanks, @ilyakuz-db.

Merging the main branch again to resolve the conflict and then this will go in.

Can you TAL at the remaining comment?

pietern · 2024-12-18T09:38:59Z

bundle/internal/schema/annotations.go

+func (d *annotationHandler) addAnnotations(typ reflect.Type, s jsonschema.Schema) jsonschema.Schema {
+	refPath := getPath(typ)
+	shouldHandle := strings.HasPrefix(refPath, "github.com")
+	if !shouldHandle {


Which types don't have this prefix?

Some primitive types like int64, string etc

github-actions · 2024-12-18T09:59:50Z

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

PR number: 1957
Commit SHA: 835fb05b873acd806cabe097770b7eb3bc18ff32

Checks will be approved automatically on success.

eng-dev-ecosystem-bot · 2024-12-18T10:01:11Z

Test Details: go/deco-tests/12390589198

## Changes I noticed that #1957 took a dep on this library even though we no longer need it. This change removes the dep and cleans up other (unused) uses of the library. We originally relied on this library to deserialize bundle configuration and JSON payloads to non-bundle CLI commands. Relevant commits: * The YAML flag was added to support apps (very early), and is not longer used: e408b70 * First use for bundle configuration loading: e47fa61 * Switch bundle configuration loading to use `libs/dyn`: 87dd46a ## Tests The build works without the dependency.

feat: Custom annotations for json-schema fields in bundle

9b588c0

ilyakuz-db temporarily deployed to test-trigger-is December 4, 2024 22:02 — with GitHub Actions Inactive

ilyakuz-db commented Dec 4, 2024

View reviewed changes

bundle/internal/schema/annotations.yml Show resolved Hide resolved

ilyakuz-db commented Dec 4, 2024

View reviewed changes

libs/jsonschema/extension.go Show resolved Hide resolved

ilyakuz-db commented Dec 4, 2024

View reviewed changes

ilyakuz-db requested review from shreyas-goenka and pietern December 4, 2024 22:08

ilyakuz-db added 2 commits December 6, 2024 15:07

feat: Introduce separate files for OpenAPI annotations

12c4373

test: Checks missing descriptions in annotation files

049e11b

ilyakuz-db temporarily deployed to test-trigger-is December 6, 2024 18:19 — with GitHub Actions Inactive

fix: Linter fix

8016d41

ilyakuz-db temporarily deployed to test-trigger-is December 6, 2024 18:22 — with GitHub Actions Inactive

fix: Remove redundant field

0221c28

ilyakuz-db temporarily deployed to test-trigger-is December 6, 2024 18:23 — with GitHub Actions Inactive

fix: Skip tests on windows

a3fdc8d

ilyakuz-db temporarily deployed to test-trigger-is December 6, 2024 18:31 — with GitHub Actions Inactive

ilyakuz-db temporarily deployed to test-trigger-is December 6, 2024 18:32 — with GitHub Actions Inactive

shreyas-goenka reviewed Dec 9, 2024

View reviewed changes

pietern reviewed Dec 9, 2024

View reviewed changes

ilyakuz-db added 5 commits December 9, 2024 12:23

fix: Run package instead of set of files

79a88e9

fix: Custom typePath function instead of exposing private `jsonschema…

c3d049f

….typePath` function

fix: Change the structure of annotation files, minor fixes and tests

b076813

feat: Add styles

073aeca

feat: Yaml styles for open api overrides

a6c45d5

pietern reviewed Dec 17, 2024

View reviewed changes

ilyakuz-db added 8 commits December 17, 2024 14:16

fix: Example structure in annotationFile comment

1b31c09

fix: Typo in assignAnnotation

7fb1ed8

fix: Genkit re-generate

9f3ec3d

fix: Remove unnecessary ok check

3231cff

fix: More explicit names for annotation data structures

c619a73

test: Test cases for markdown link converter

3049184

fix: More explicit names and descriptions for `annotationHandler.sync…

0037671

…` method

fix: Use OS-aware path joiner

7d8ae48

ilyakuz-db temporarily deployed to test-trigger-is December 17, 2024 14:07 — with GitHub Actions Inactive

ilyakuz-db temporarily deployed to test-trigger-is December 17, 2024 14:08 — with GitHub Actions Inactive

ilyakuz-db added 2 commits December 17, 2024 16:43

fix: Update test to use dynamic values for comparison instead of raw …

34d7484

…file content

Merge branch 'main' of github.com:databricks/cli into feat/custom-ann…

386c7d3

…otations-json-schema

ilyakuz-db temporarily deployed to test-trigger-is December 17, 2024 15:48 — with GitHub Actions Inactive

ilyakuz-db temporarily deployed to test-trigger-is December 17, 2024 15:49 — with GitHub Actions Inactive

ilyakuz-db requested a review from pietern December 17, 2024 16:05

pietern changed the title ~~feat: Custom annotations for json-schema fields in bundle~~ Custom annotations for bundle-specific JSON schema fields Dec 18, 2024

pietern approved these changes Dec 18, 2024

View reviewed changes

Merge branch 'main' into feat/custom-annotations-json-schema

835fb05

pietern temporarily deployed to test-trigger-is December 18, 2024 09:59 — with GitHub Actions Inactive

pietern enabled auto-merge December 18, 2024 10:00

pietern added this pull request to the merge queue Dec 18, 2024

Merged via the queue into main with commit 042c8d8 Dec 18, 2024
10 checks passed

pietern deleted the feat/custom-annotations-json-schema branch December 18, 2024 10:25

pietern mentioned this pull request Dec 18, 2024

Remove dependency on ghodss/yaml #2032

Merged

	assingAnnotation(&s, rootTypeAnnotation)
	assignAnnotation(&s, rootTypeAnnotation)

Custom annotations for bundle-specific JSON schema fields #1957

Custom annotations for bundle-specific JSON schema fields #1957

Conversation

ilyakuz-db commented Dec 4, 2024 • edited Loading

Changes

Tests

ilyakuz-db Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyas-goenka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyas-goenka Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

ilyakuz-db commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyakuz-db Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 18, 2024

eng-dev-ecosystem-bot commented Dec 18, 2024

ilyakuz-db commented Dec 4, 2024 •

edited

Loading

ilyakuz-db Dec 4, 2024 •

edited

Loading

shreyas-goenka Dec 9, 2024 •

edited

Loading

ilyakuz-db Dec 17, 2024 •

edited

Loading