Skip to content
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

[DOC][SQL JDBC]change setup URL #38564

Merged
merged 4 commits into from
Feb 14, 2019
Merged

[DOC][SQL JDBC]change setup URL #38564

merged 4 commits into from
Feb 14, 2019

Conversation

taku333
Copy link

@taku333 taku333 commented Feb 7, 2019

I read the following sources and thought that it was necessary to change the documentation of [SQL JDBC].
change setup URL「?」→「://」

https://github.com/elastic/elasticsearch/blob/6857d305270be3d987689fda37cc84b7bc18fbb3/x-pack/plugin/sql/qa/security/src/test/java/org/elasticsearch/xpack/sql/qa/security/JdbcSecurityIT.java

@polyfractal polyfractal added >docs General docs changes :Analytics/SQL SQL querying labels Feb 7, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@costin
Copy link
Member

costin commented Feb 12, 2019

Thank you for your PR. The fix is partially correct: the extra :// are needed only when the scheme is needed which, itself is optional.
The correct pattern should be:

jdbc:es://<1>[[http|https]://]*<2>[host[:port]]*<3>/[prefix]*<4>[?[option=value]&<5>]*

Can you please adjust accordingly?

Thank you.

taku333 added 2 commits February 13, 2019 10:59
change setup URL Option 「[http|https]://」→「[[http|https]://]」
change setup URL Option Reflect error.
@taku333
Copy link
Author

taku333 commented Feb 13, 2019

@costin
Thank you for review.
Fixed the document.
Please check again.

Copy link

@Blackiie Blackiie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅

@astefan
Copy link
Contributor

astefan commented Feb 13, 2019

@taku333 due to how Markdown rendered @costin's URL, the first asterix was not visible (making http|https:// optional). I've edited @costin's comment and now it is shown. Can you, please, make a final change to the documentation according to the edited post? Thanks.

change setup URL Option Reflect error.
@taku333
Copy link
Author

taku333 commented Feb 13, 2019

@astefan @costin
thanks! I edited again.
Please confirm.

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@astefan astefan merged commit 103786e into elastic:6.6 Feb 14, 2019
astefan pushed a commit that referenced this pull request Feb 14, 2019
@taku333 taku333 deleted the patch-2 branch February 14, 2019 12:42
astefan pushed a commit that referenced this pull request Feb 14, 2019
astefan pushed a commit that referenced this pull request Feb 14, 2019
astefan pushed a commit that referenced this pull request Feb 14, 2019
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 14, 2019
* elastic/master:
  Remove immediate operation retry after mapping update (elastic#38873)
  Remove mentioning of types from bulk API docs (elastic#38896)
  SQL: change JDBC setup URL in the documentation (elastic#38564)
  Skip BWC tests in checkPart1 and checkPart2 (elastic#38730)
  Enable silent FollowersCheckerTest (elastic#38851)
  Update TESTING.asciidoc with platform specific instructions (elastic#38802)
  Use consistent view of realms for authentication (elastic#38815)
  Stabilize RareClusterState (elastic#38671)
  Increase Timeout in UnicastZenPingTests (elastic#38893)
  Do not recommend installing vagrant-winrm elastic#38887
  _cat/indices with Security, hide names when wildcard (elastic#38824)
  SQL: fall back to using the field name for column label (elastic#38842)
  Fix LocalIndexFollowingIT#testRemoveRemoteConnection() test (elastic#38709)
  Remove joda time mentions in documentation (elastic#38720)
  Add enabled status for token and api key service (elastic#38687)
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants