Skip to content

feat(clickhouse)!: Read only plain ca_cert#9495

Merged
kodiakhq[bot] merged 1 commit into
mainfrom
feat/ch/plain-ca
Mar 29, 2023
Merged

feat(clickhouse)!: Read only plain ca_cert#9495
kodiakhq[bot] merged 1 commit into
mainfrom
feat/ch/plain-ca

Conversation

@candiduslynx
Copy link
Copy Markdown
Contributor

@candiduslynx candiduslynx commented Mar 29, 2023 •
edited
Loading

BEGIN_COMMIT_OVERRIDE
feat(clickhouse)!: Read only plain ca_cert value (#9495)

BREAKING-CHANGE: Stop reading ca_cert value as file path. See file variable substitution for how to read this value from a file.
END_COMMIT_OVERRIDE

@candiduslynx candiduslynx added the automerge Automatically merge once required checks pass label Mar 29, 2023
@candiduslynx candiduslynx requested review from a team, erezrokah, hermanschaaf and yevgenypats and removed request for a team and yevgenypats March 29, 2023 06:28
@candiduslynx candiduslynx changed the title fix(clickhouse): ca_cert fixes: load only as plaintext & check append result fix(clickhouse): Check ca_cert append result Mar 29, 2023
erezrokah
erezrokah reviewed Mar 29, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@erezrokah erezrokah left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @candiduslynx, can you explain what is the bug here? Are we removing the file handling for using the built in SDK file expanding? Isn't this a breaking change? Or this didn't work at all?

Also, maybe provide some examples in ca_cert to how the value(s) should look like.

@candiduslynx
Copy link
Copy Markdown
Contributor Author

candiduslynx commented Mar 29, 2023

@erezrokah this PR has 2 changes (maybe they should be separate):

  1. Check the AppendCertsFromPEM returned value to determine if the cert provided is valid
  2. Remove the read file logic to use the built-in SDK file expansion

Possibly, the 2nd change should be expanded into a separate PR marked breaking (although I doubt anyone used this, as it required mounting the custom cert file to the container).

@candiduslynx candiduslynx requested a review from erezrokah March 29, 2023 09:27
@erezrokah
Copy link
Copy Markdown
Member

erezrokah commented Mar 29, 2023 •
edited
Loading

Possibly, the 2nd change should be expanded into a separate PR marked breaking (although I doubt anyone used this, as it required mounting the custom cert file to the container).

I think we should play it safe and do it as a breaking change + provide a change log entry that explains how to use the file placeholder in case someone needs to migrate, WDYT?

@candiduslynx
Copy link
Copy Markdown
Contributor Author

candiduslynx commented Mar 29, 2023

OK, I'll extract the breaking part into a separate PR and leave this PR to address the AppendCertsFromPEM returned value.

@candiduslynx candiduslynx mentioned this pull request Mar 29, 2023
Merged
@candiduslynx candiduslynx force-pushed the feat/ch/plain-ca branch 2 times, most recently from 14c467d to b7990c6 Compare March 29, 2023 12:06
@candiduslynx candiduslynx changed the title fix(clickhouse): Check ca_cert append result feat(clickhouse)!: Read only plain ca_cert Mar 29, 2023
kodiakhq Bot pushed a commit that referenced this pull request Mar 29, 2023
@candiduslynx
fix(clickhouse): Check ca_cert append result (#9505)
eea1b11 
Extracted from #9495
@candiduslynx
Copy link
Copy Markdown
Contributor Author

candiduslynx commented Mar 29, 2023

@erezrokah I've updated the PR & the description.
Kindly let me know if anything additional is required.

erezrokah
erezrokah approved these changes Mar 29, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

Thanks @candiduslynx, looks great 🚀

@kodiakhq kodiakhq Bot merged commit dcffd50 into main Mar 29, 2023
@kodiakhq kodiakhq Bot deleted the feat/ch/plain-ca branch March 29, 2023 12:31
@cq-bot cq-bot mentioned this pull request Mar 29, 2023
Merged
kodiakhq Bot pushed a commit that referenced this pull request Apr 4, 2023
@cq-bot
chore(main): Release plugins-destination-clickhouse v2.0.0 (#9507)
223cbda 
🤖 I have created a release *beep* *boop*
---


## [2.0.0](plugins-destination-clickhouse-v1.3.3...plugins-destination-clickhouse-v2.0.0) (2023-04-04)


### ⚠ BREAKING CHANGES

* **clickhouse:** Stop reading `ca_cert` value as file path. See [file variable substitution](/docs/advanced-topics/environment-variable-substitution#file-variable-substitution-example) for how to read this value from a file.

### Features

* **clickhouse:** Read only plain `ca_cert` value ([#9495](#9495)) ([dcffd50](dcffd50))


### Bug Fixes

* **clickhouse:** Check `ca_cert` append result ([#9505](#9505)) ([eea1b11](eea1b11))
* **deps:** Update golang.org/x/exp digest to 10a5072 ([#9587](#9587)) ([31f913f](31f913f))
* **deps:** Update module github.com/cloudquery/plugin-sdk to v1.44.1 ([#9520](#9520)) ([202c31b](202c31b))
* **deps:** Update module github.com/cloudquery/plugin-sdk to v1.44.2 ([#9661](#9661)) ([a27dc84](a27dc84))
* **deps:** Update module github.com/mattn/go-isatty to v0.0.18 ([#9609](#9609)) ([5b2908e](5b2908e))
* Ignore primary key option when migrating tables ([3a0c68b](3a0c68b))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erezrokah erezrokah erezrokah approved these changes

@hermanschaaf hermanschaaf Awaiting requested review from hermanschaaf

Assignees

No one assigned

Labels

automerge Automatically merge once required checks pass

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@candiduslynx @erezrokah @cq-bot