-
Notifications
You must be signed in to change notification settings - Fork 5.5k
WIP: Validating config in docs #11394
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
Changes from 2 commits
97e003b
4975c02
906ff63
6b92ccb
204428d
382c36d
c7bdb31
881e654
2bbe15c
0f0446b
071de20
ee0ef1d
3312681
77b7b75
4f0aecd
1fb3282
8619b7a
6c13087
a60d3ff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| from typing import List | ||
| from docutils import nodes | ||
| from docutils.parsers.rst import Directive | ||
| from docutils.parsers.rst import directives | ||
| from sphinx.application import Sphinx | ||
| from sphinx.util.docutils import SphinxDirective | ||
| from sphinx.directives.code import CodeBlock | ||
| from sphinx.errors import ExtensionError | ||
|
|
||
| import subprocess | ||
|
|
||
| class ValidatingCodeBlock(CodeBlock): | ||
|
|
||
| has_content = True | ||
| required_arguments = CodeBlock.required_arguments | ||
| optional_arguments = CodeBlock.optional_arguments | ||
| final_argument_whitespace = CodeBlock.final_argument_whitespace | ||
| option_spec = { | ||
| 'type-name': directives.unchanged, | ||
| } | ||
| option_spec.update(CodeBlock.option_spec) | ||
|
|
||
| def run(self): | ||
| source, line = self.state_machine.get_source_and_line(self.lineno) | ||
| # built-in directives.unchanged_required option validator produces a confusing error message | ||
| if (self.options.get('type-name') == None): | ||
|
dmitri-d marked this conversation as resolved.
Outdated
|
||
| raise ExtensionError("Expected type name in: {0} line: {1}".format(source, line)) | ||
|
|
||
| process = subprocess.Popen(['bazel', 'run', '//tools/config_validation:validate_fragment', '--', self.options.get('type-name'), "-s", "\n".join(self.content)], | ||
|
dmitri-d marked this conversation as resolved.
Outdated
dmitri-d marked this conversation as resolved.
Outdated
|
||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE) | ||
| stdout, stderr = process.communicate() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks really nice and clean. You mention on Slack that this is pretty slow (around 10s), which won't scale. You mentioned a "customer builder", curious to learn more. If you want to continue with your existing PR, I'd suggest that you build a That might still end up being too slow, e.g. if it takes ~1s and you have 100, that's over a minute and a half. When I would do then is add an env var to control validation. In CI we would always enable, but for local iterations on docs builds, we could disable.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
My thinking is that we'd use a dedicated pass to verify example configs by invoking a dedicated builder. All it would do is to validate configs and generate a report. When a "normal" builder is used, examples would still be rendered, but without validation. Basically we'd be doing config validation in one batch and it would be explicitly invoked. Current implementation combines doc generation and validation, and I'm not sure I can/should continue on validation errors: I think putting a global state into a directive (which is what
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to failing hard on validation errors. How about this for an idea..
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think what you are suggesting is quite close to a dedicated builder, it would qork quite similar to what you have described. This is the way to go If we want to validate all config examples and then report the ones that failed (as opposed to stopping at the first failure, like it's currently implemented).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think the other advantage is pure speed. Right now, you are invoking Bazel and Python multiple times to be able to do the validation. I'm guessing only a small fraction of CPU cycles are spent actually in the validation (could be worth measuring). |
||
| if (process.poll()): | ||
| raise ExtensionError("Failed config validation for type: '{0}' in: {1} line: {2}".format(self.options.get('type-name'), source, line)) | ||
|
|
||
| self.options.pop('type-name', None) | ||
| return list(CodeBlock.run(self)) | ||
|
|
||
| def setup(app): | ||
| app.add_directive("validated-code-block", ValidatingCodeBlock) | ||
|
|
||
| return { | ||
| 'version': '0.1', | ||
| 'parallel_read_safe': True, | ||
| 'parallel_write_safe': True, | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.