ESQL: Fail build docs aren't up to date #131739
Conversation
Adds a new behavior to the esql build we enable in CI that, instead of generating the docs, asserts that they are already up to date.
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
🔍 Preview links for changed docs |
There was a problem hiding this comment.
It would be nice if we could just do a git diff after generation, to avoid propagating that logic to the code. Easier to maintain also, IMO.
I see only the build-tools module depends on git though, but I would expect git to be available in both CI and machines running test
There was a problem hiding this comment.
I was trying to keep as much stuff as I could out of the build.gradle so we can maintain it ourselves. And there's some trickiness with the replacements.... But I did think about it.
| } | ||
| if (renderedLines.size() < found.size()) { | ||
| if (renderedLines.size() + 1 == found.size() && found.get(renderedLines.size()).isEmpty()) { | ||
| // trailing newline, whatever |
There was a problem hiding this comment.
This means somebody else will have the change in their PRs anyway right? I would probably start with a hard equals, unless you found some edge case like that already
There was a problem hiding this comment.
I had seen newlines at first. Let me double check.
|
|
||
| @Override | ||
| public boolean supportsRendering() { | ||
| // CI doesn't always support rendering the svgs. And we hack them anyway so they don't match. |
There was a problem hiding this comment.
What is specifically unsupported of SVGs in CI? Isn't the library "just" crafting the XML? Or is it doing something else?
There was a problem hiding this comment.
It has to render the font to get some pixel values. I know, it shouldn't have to do that, but it does! I can expand the comment.
There was a problem hiding this comment.
And most CI machines are fine with it. But some are missing the components to make the render fonts at all. The build provides the font too.
| public static class AssertCallbacks implements Callbacks { | ||
| @Override | ||
| public void write(Path dir, String name, String extension, String str, boolean kibana) throws IOException { | ||
| Path file = dir.resolve(name + "." + extension); |
There was a problem hiding this comment.
What about adding a message like "Run the changes function tests to regenerate the docs, and push them"? As to guide devs for their first contributions, and avoid the sense of flakyness in tests.
Maybe just try-catching the code of this method and wrapping the exception
There was a problem hiding this comment.
Yeah. I think that's a good choice.
3 participants
Adds a new behavior to the esql build we enable in CI that, instead of generating the docs, asserts that they are already up to date.