-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Initial implementation of DbUp.ClickHouse #2
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
Conversation
|
@droyad is there anything I can do to help you with this PR? |
There was a problem hiding this 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> |
There was a problem hiding this comment.
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
| <TargetFrameworks>netstandard2.1;net462;net48;net8.0;net9.0</TargetFrameworks> | |
| <TargetFrameworks>netstandard2.1;net462</TargetFrameworks> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| <AssemblyOriginatorKeyFile>../dbup.snk</AssemblyOriginatorKeyFile> | ||
| <SignAssembly>true</SignAssembly> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
See @droyad comment on PR DbUp#2 (comment)
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
|
I'll publish tomorrow. There are some other maintenance things that should go out at the same time. |
|
Took me a bit to get the build working. It's not quite there, but enough to at least merge this PR. |
|
It has been released! @shokurov Are you interested in being the maintainer for this provider? |
|
Yes, I am using it in production anyway. Will be glad to help. |
Checklist
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.