Skip to content

Conversation

@shokurov
Copy link
Contributor

@shokurov shokurov commented Sep 9, 2025

Checklist

  • I have read the Contributing Guide
  • I have checked to ensure this does not introduce an unintended breaking changes
  • I have considered appropriate testing for my change

Description

Initial implementation based on official ClickHouse.Driver, with proper test coverage.

Additionally I've create integration tests to run over TestContainers.ClickHouse. I'm not sure if this feasible for Github pipelines, but its totally worth having in local development.

@shokurov
Copy link
Contributor Author

@droyad is there anything I can do to help you with this PR?

Copy link
Member

@droyad droyad left a comment

Choose a reason for hiding this comment

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

A small change and question

<Product>DbUp</Product>
<Copyright>Copyright © DbUp Contributors 2015</Copyright>
<TargetFramework>netstandard2.0</TargetFramework>
<TargetFrameworks>netstandard2.1;net462;net48;net8.0;net9.0</TargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we can't just target these two? It covers all frameworks listed

Suggested change
<TargetFrameworks>netstandard2.1;net462;net48;net8.0;net9.0</TargetFrameworks>
<TargetFrameworks>netstandard2.1;net462</TargetFrameworks>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Frankly I copied the whole target list from Clickhouse.Driver and I did not really want to get into the details on what optimizations they might have done for all these targets. @droyad do you think a shorter list has its advantages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@droyad I think you are right to question, the shorter list the less package size, and the Clickhouse.Driver itself will be linked with proper optimization regardless of dbup-clickhouse`s own target. Removed the excess

Comment on lines -13 to -14
<AssemblyOriginatorKeyFile>../dbup.snk</AssemblyOriginatorKeyFile>
<SignAssembly>true</SignAssembly>
Copy link
Member

Choose a reason for hiding this comment

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

This should stay as it helps it be whitelisted and not picked up by AV

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, restored that, thanks for noticing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops.. We've had

error CS8002: Referenced assembly 'ClickHouse.Driver, Version=0.7.20.0, Culture=neutral, PublicKeyToken=null' does not have a strong name

Reverted it

Copy link
Member

Choose a reason for hiding this comment

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

Ugh, so annoying. Thanks for checking.

Assembly signing failed because of
error CS8002: Referenced assembly 'ClickHouse.Driver, Version=0.7.20.0, Culture=neutral, PublicKeyToken=null' does not have a strong name
droyad
droyad previously approved these changes Nov 5, 2025
@droyad droyad enabled auto-merge (squash) November 5, 2025 08:20
@droyad
Copy link
Member

droyad commented Nov 5, 2025

I'll publish tomorrow. There are some other maintenance things that should go out at the same time.

@droyad
Copy link
Member

droyad commented Nov 6, 2025

Took me a bit to get the build working. It's not quite there, but enough to at least merge this PR.

@droyad droyad disabled auto-merge November 6, 2025 01:47
@droyad droyad merged commit 2fc5caf into DbUp:main Nov 6, 2025
1 check failed
@droyad
Copy link
Member

droyad commented Nov 6, 2025

It has been released!

@shokurov Are you interested in being the maintainer for this provider?

@shokurov
Copy link
Contributor Author

shokurov commented Nov 6, 2025

Yes, I am using it in production anyway. Will be glad to help.

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.

3 participants