-
Notifications
You must be signed in to change notification settings - Fork 123
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
320: add version to _dlt_loads table #466
Conversation
✅ Deploy Preview for dlt-hub-docs canceled.
|
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.
code changes are solid and all is ok there! thanks!
we still need to improve some tests. I enabled CI for your fork and a few tests are not passing
- small linter issue (if you have poetry and make you can just do
make dev && make lint
) - common test problems: https://github.com/dlt-hub/dlt/actions/runs/5430315589/jobs/9969881019?pr=466
why the common tests are failing: each schema has a content hash used to detect schema changes. increasing the version changed the hash (when I load fixtures in tests, your migration modifies the content and we get a different hash - then tests that check hashes fail)
to fix: we need a v6 version of this fixture: ethereum_schema_v5.yml. I generate it with a dlt command:
dlt schema tests/common/cases/schemas/eth/ethereum_schema_v5.yml --remove-defaults
and save the output to new name. from it you can get the new content hash and put it into IMPORTED_VERSION_HASH_ETH_V5 constant (used by the tests)
btw. we'll upgrade our CI to be more friendly for PRs from forks: #476
if the above looks too complicated or unclear just ping us, someone will take over the final fixes
aef5e5f
to
73db171
Compare
Hi @rudolfix Thanks a lot for your comments :) I took a stab at them and things look good locally. However, the nefly CI seems to be blocking the test from running on my fork. Is there anything I can do about that 🤔 ? |
@codingcyclist I just unblocked CI. again... maybe it blocks PRs after some inactivity. will check out |
73db171
to
ba94e54
Compare
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.
LGTM!
Resolves 320