-
Notifications
You must be signed in to change notification settings - Fork 69
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
Autogenerated documentation for bundle config #2033
Conversation
Test Details: go/deco-tests/12401106836 |
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
Makefile
Outdated
schema: | ||
go run ./bundle/internal/schema ./bundle/internal/schema ./bundle/schema/jsonschema.json | ||
|
||
docs: vendor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs: vendor
We don't add this implicitly anymore to other targets. If you're on CI, you can always specify both with "make vendor docs".
BTW, do you intended to run it on CI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, do you intended to run it on CI?
Currently the only use case of using that tool is to checkout CLI repo and run locally
We don't add this implicitly anymore to other targets
By implicitly you mean using make deps or in general?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the make entry should be:
docs:
...
no dependency on vendor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate little bit more on the reasoning behind "We don't add this implicitly anymore to other targets" so I can understand how to better fix it?
Is it just that we don't want to rely on make deps syntax and this is still allowed?
docs:
make vendor
go run ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed and update the readme
Makefile
Outdated
schema: | ||
go run ./bundle/internal/schema ./bundle/internal/schema ./bundle/schema/jsonschema.json | ||
|
||
docs: vendor | ||
go run ./bundle/internal/docs ./bundle/internal/schema ./bundle/internal/docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this part if /bundle/internal/ ?
internal is meant for code that is not meant to be used outside of the package, but this is clearly intended to be used outside the package, that's why it's in Makefile.
How about placing this at top level? /docsgen/ ?
I see you're following existing structure, so it's a more of a question for @pietern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you're following existing structure,
Yes I reused same approach as with json schema generation which is used in .codegen.json
along with other internal scripts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is bundle specific, so bundle/docsgen
would be more appropriate.
It doesn't need to be internal.
) | ||
|
||
// Parsed file with annotations, expected format: | ||
// github.com/databricks/cli/bundle/config.Bundle: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is copied, right?
It would be much easier to review if PRs was split into
- refactoring: copy / renames
- new functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this was moved as is from schema
package into the new package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be much easier to review if PRs was split into
refactoring: copy / renames
new functionality
Agree but it feels too painful to split now. For next PRs I'll try to follow more convenient approach
|
||
// Examples of the value for properties in the schema. | ||
// https://json-schema.org/understanding-json-schema/reference/annotations | ||
Examples []any `json:"examples,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if the schema check we perform also validates that entries here match the containing schema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it only tests yamls against the schema but not the schema consistency itsef
In general spec says that example values should validate against schema but it isn't used and not required
https://json-schema.org/understanding-json-schema/reference/annotations see draft 6
Worth to mention that this field currently is not used in schema generation
Makefile
Outdated
schema: | ||
go run ./bundle/internal/schema ./bundle/internal/schema ./bundle/schema/jsonschema.json | ||
|
||
docs: vendor | ||
go run ./bundle/internal/docs ./bundle/internal/schema ./bundle/internal/docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is bundle specific, so bundle/docsgen
would be more appropriate.
It doesn't need to be internal.
Changes
Documentation autogeneration tool. This tool uses same annotations_*.yml files as in json-schema
Result will go there and there
Tests
Manually