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

add a note about SSL with JDBC #331

Merged
merged 4 commits into from
Jan 8, 2018
Merged

Conversation

cotedm
Copy link

@cotedm cotedm commented Dec 29, 2017

This is to update the docs to explain SSL config a bit more for this connector.

will need to configure SSL via the ``connection.url`` parameter. For example, with mysql it would
look like:

..sourcecode bash:
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use properties instead of bash for the highlighting if you want. LGTM

@jcustenborder
Copy link
Contributor

@cotedm One small thing but LGTM.

Copy link
Contributor

@ewencp ewencp left a comment

Choose a reason for hiding this comment

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

couple of nits and potential improvements, but lgtm. besides trivial changes, could merge and leave rest as follow ups.


In the connector configuration you will notice there are no security parameters. This is because
SSL is not part of the JDBC standard and will depend on the JDBC driver in use. In general, you
will need to configure SSL via the ``connection.url`` parameter. For example, with mysql it would
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: mysql should either be monotype font to indicate protocol in the connection url or MySQL to match the product name

@@ -3,6 +3,19 @@
JDBC Sink Configuration Options
-------------------------------

Security
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe emphasize which connection this security refers to? Kafka security is via worker config, but new users may not always realize that. maybe "Database Connection Security" or something like that?

Security
^^^^^^^^

In the connector configuration you will notice there are no security parameters. This is because
Copy link
Contributor

Choose a reason for hiding this comment

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

if you're going to repeat text, it is possible to include the same text via an include. @joel-hamill recently started using these to make repeated text more manageable. seems reasonable to do here, and he might have more guidance on how it makes best sense to manage this going forward.

Copy link
Author

Choose a reason for hiding this comment

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

yeah this makes sense, I gave the include directive a shot in the latest commit. I can already see just in making a couple of changes how mistake prone this repeated text would be if copied in multiple places. If it looks ok to @joel-hamill then I'll merge.

@cotedm
Copy link
Author

cotedm commented Jan 2, 2018

@joel-hamill I added you as a reviewer to make sure I'm using this include directive like you would expect for consistency.

Copy link
Member

@rhauch rhauch left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

Copy link
Contributor

@joel-hamill joel-hamill left a comment

Choose a reason for hiding this comment

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

See includes guidance

@@ -0,0 +1,12 @@
Database Connection Security
Copy link
Contributor

@joel-hamill joel-hamill Jan 2, 2018

Choose a reason for hiding this comment

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

@cotedm for the include, the convention is to nest in an /includes folder (e.g. connect-jdbc/docs/includes/db_connection_security.rst). also, not an issue here, but eventually if you have more than one string to include, you can do this by referencing the specific line numbers for your include (see https://github.com/confluentinc/docs#content-reuse).

Copy link
Contributor

Choose a reason for hiding this comment

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

i've added more explanation here as well https://github.com/confluentinc/docs/pull/650

Copy link
Author

Choose a reason for hiding this comment

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

thanks @joel-hamill! how's this look? If it's good, I think we're ok to merge :)

Copy link
Contributor

@ewencp ewencp left a comment

Choose a reason for hiding this comment

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

one final formatting issue, but the include setup looks good now

will need to configure SSL via the ``connection.url`` parameter. For example, with MySQL it would
look like:

..sourcecode properties:
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be .. sourceode:: properties. right now it is rendering weirdly because it's not being interpreted as an rst command

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, the output is messed up
image

Copy link
Contributor

@joel-hamill joel-hamill left a comment

Choose a reason for hiding this comment

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

Formatting is off

will need to configure SSL via the ``connection.url`` parameter. For example, with MySQL it would
look like:

..sourcecode properties:
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, the output is messed up
image

@ewencp
Copy link
Contributor

ewencp commented Jan 3, 2018

@joel-hamill I pushed a fix for the formatting issue if you want to verify. Then I think we're good to merge.

Copy link
Member

@kkonstantine kkonstantine left a comment

Choose a reason for hiding this comment

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

LGTM!

@cotedm cotedm merged commit 88cc1b4 into confluentinc:3.3.1-post Jan 8, 2018
@cotedm cotedm deleted the jdbc-and-ssl branch January 8, 2018 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants