Skip to content

Conversation

@SlNPacifist
Copy link

This allows using clickhouse installations with self-signed certificates via rejectUnauthorized: false option or by setting correct ca option.

@codecov-io
Copy link

codecov-io commented May 24, 2019

Codecov Report

Merging #35 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #35   +/-   ##
=======================================
  Coverage   88.65%   88.65%           
=======================================
  Files           6        6           
  Lines         326      326           
=======================================
  Hits          289      289           
  Misses         37       37
Impacted Files Coverage Δ
src/clickhouse.js 97.56% <100%> (ø) ⬆️
src/consts.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e004e6a...cadcebd. Read the comment docs.

@nezed
Copy link
Collaborator

nezed commented May 26, 2019

Thanks for your contribution!

Since its not a first issue with missing http options (See #32) i've changed logic of options providing from "whitelist" to "not in blacklist".

BTW, did you triend NODE_EXTRA_CA_CERTS env with node@7+ ?
https://nodejs.org/dist/latest/docs/api/cli.html#cli_node_extra_ca_certs_file

@nezed nezed force-pushed the node-https-options branch 2 times, most recently from 34c5675 to b6865b8 Compare May 26, 2019 14:22
@nezed nezed added this to the 2.0.0 milestone May 26, 2019
@SlNPacifist
Copy link
Author

Thanks for fast reaction!
I'm ok with any solution that will allow setting rejectUnauthorized: false.

I did not try NODE_EXTRA_CA_CERTS since my code will be running on ~100 developers' PCs and they do not have this env variable for sure. I could run a subprocess with needed env variable but this is an overkill.

You can drop my commit & close PR since it does not contribute to the final code. I think this is better for repo history. Waiting for 2.0.0!

@nezed
Copy link
Collaborator

nezed commented May 26, 2019

I think there is no breaking changes, so i'm trying asking @apla to bump patch with this PR 🙃

@SlNPacifist
Copy link
Author

@apla Could you merge or discard this PR please?

@nezed nezed removed this from the 2.0.0 (Minimal breaking changes) milestone Jun 1, 2019
@apla
Copy link
Owner

apla commented Jun 1, 2019

Sorry, but I don't want to use anything with lodash in name, especially omit. Please, just use filter.

@nezed
Copy link
Collaborator

nezed commented Jun 1, 2019

We can solve it with rest params operator when #38 will be finished.
Just like it was solved in issue that you provided FormidableLabs/victory-chart#577

@SlNPacifist SlNPacifist force-pushed the node-https-options branch 3 times, most recently from f92189d to 17e1fb9 Compare June 2, 2019 10:47
@SlNPacifist
Copy link
Author

@apla fixed. Take a look please.

@SlNPacifist
Copy link
Author

@apla Any chances this PR gets accepted?

@nezed nezed force-pushed the node-https-options branch from 17e1fb9 to cadcebd Compare October 4, 2019 21:57
@nezed nezed merged commit cc30b25 into apla:master Oct 4, 2019
@nezed
Copy link
Collaborator

nezed commented Oct 4, 2019

@SlNPacifist Now i have permissions to bump new versions.
Your fix is now published as @apla/clickhouse@1.6.1, so you can switch back from your fork 🙃

My apologies for this situation, i've done my best 😔

Check out https://github.com/apla/node-clickhouse/releases/tag/v1.6.1

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.

4 participants