-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Fix the SQL docs for SHOW TABLES have misaligned examples #115285
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
base: main
Are you sure you want to change the base?
Conversation
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Pinging @elastic/es-docs (Team:Docs) |
run docs-build |
@elasticmachine test this |
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.
Thank you @jackpan123 , that's certainly an improvement!
However, I think we should also update all .csv-spec
files inside the following directories (offending tests can be found by looking for javaRestTest
):
x-pack/plugin/sql/qa/server/src/main/resources/docs/
x-pack/plugin/sql/qa/server/src/main/resources/multi-cluster-with-security
This should cover all the csv-spec files that are used in docs. (I ran $ rgrep 'tag::' .
to find csv tests that are tagged and can thus be included in docs.)
Would you mind taking care of that? Then I think we'll be able to merge this.
By the way: we have hundreds of other misaligned tests - all of them have in common that they use javaRestTest
as cluster name instead of testCluster
. A quick $ rgrep javaRestTest .
inside x-pack/plugin/sql/qa/server/src/main/resources
shows all potential relevant places.
@leemthompo , if you think javaRestTest
being used as cluster name is too unfitting for the docs, we need to solve this differently. The cause for javaRestTest
being used seems to be #86626 . A nicer solution would be to go and update our integration tests to manually set the cluster name to testCluster
. But that would be much more effort.
Thanks for the additional context @alex-spies |
Important Elastic documentation is migrating to Markdown for version 9.0+. See the migration guide for details. ℹ️ What's happening?
What do I need to do?For <=8.x docs:
For 9.0+ docs:Option 1:
Option 2:
💡 Need help?
|
@alex-spies I have already fixed all the |
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’ve fixed all the .csv-spec files
for javaRestTest
.
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.
Sorry to keep you waiting @jackpan123 . Now that our docs migration is done, I think we can move forward.
While I can still see some tests with javaRestTest
being misaligned, these aren't tagged and thus aren't used in any docs. This PR helps align the tests that are being used in docs and is thus a good addition to our 8.19/8.18/8.17 docs.
As @leemthompo mentioned, unfortunately, this PR cannot go into our main
branch as that's using a different doc format now altogether.
@jackpan123 , rebasing this onto the 8.19
branch may be tricky, but if you're still up for it, you could create another PR that branches off from the 8.19 branch and cherry-picks the 2 commits that contain changes here?
The changes themselves LGTM - we only need to target the correct branch.
Fix the SQL docs for SHOW TABLES have misaligned examples (Closes #102565 )