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

Remove hostname and port from JDBC examples to avoid confus… #1786

Merged
merged 3 commits into from
Oct 1, 2019

Conversation

bsideup
Copy link
Member

@bsideup bsideup commented Aug 25, 2019

Since they are ignored, it is better to not have any defaults to avoid confusing the users. ///databasename-style URLs work great and raise less questions IMO :)

@bsideup bsideup requested a review from a team August 25, 2019 16:18

Insert `tc:` after `jdbc:` as follows. Note that the hostname, port and database name will be ignored; you can leave these as-is or set them to any value.

### JDBC URL examples

#### Using Testcontainers with a fixed version

`jdbc:tc:mysql:5.6.23://somehostname:someport/databasename`
`jdbc:tc:mysql:5.6.23:///databasename`
Copy link
Member

Choose a reason for hiding this comment

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

I suspect that many people will be so used to seeing hostname:port parts in JDBC URLs that it'll be quite surprising to see them omitted - i.e. it will look unfamiliar to the point of being weird.

Could we perhaps have a 'note' callout under line 52 explaining and showing that scheme://foo:bar/baz and scheme:///baz are equivalent?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem I seen was people putting localhost:3306 there and expecting the DB to actually run on these host/port (by attaching MySQL Workbench or something else).

This magical /// can be a good indicator that the host/port are unknown and will be magically set later. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I'm totally in favour of this PR, but I think we need to explain or reword line 52 at a minimum.

I'd be willing to bet that many people haven't made the connection in their heads that host-less URIs are a thing (despite the existence of file URIs!).

Copy link
Member Author

Choose a reason for hiding this comment

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

updated 👍

@rnorth rnorth added this to the next milestone Sep 8, 2019
Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

Looks good to me - thank you 😄

@rnorth rnorth changed the title Remove hostname and port from JDBC examples to avoid confusion Remove hostname and port from JDBC examples to avoid confus… Oct 1, 2019
@rnorth rnorth merged commit 9ab6909 into master Oct 1, 2019
@rnorth rnorth deleted the remove_hostname_and_port_from_jdbc_docs branch October 1, 2019 19:58
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.

None yet

2 participants