apla / node-clickhouse

Yandex ClickHouse driver for nodejs

MIT License

216 stars 50 forks source link

All existing nodejs http & https request options are now allowed #35

Closed SlNPacifist closed 4 years ago

SlNPacifist commented 5 years ago

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

codecov-io commented 5 years ago

Codecov Report

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

@@           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%> (ø)`	:arrow_up:
src/consts.js	`100% <100%> (ø)`	:arrow_up:

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 commented 5 years ago

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

SlNPacifist commented 5 years ago

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 commented 5 years ago

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

SlNPacifist commented 5 years ago

@apla Could you merge or discard this PR please?

apla commented 5 years ago

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

nezed commented 5 years ago

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

SlNPacifist commented 5 years ago

@apla fixed. Take a look please.

SlNPacifist commented 5 years ago

@apla Any chances this PR gets accepted?

nezed commented 4 years ago

@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