Skip to content
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

vdk-impala: Introduce checks for scd1 template #1472

Merged

Conversation

sbuldeev
Copy link
Collaborator

@sbuldeev sbuldeev commented Jan 4, 2023

What:
Adding functionality to handle the scd1 template behavior if there are checks provided by the user

Why:
It is linked to #1361

Tests: provided positive and negative regression tests

What:
Adding functionality to handle the scd1 template behavior if there are checks provided by the user

Why:
It is linked to #1361

Tests: provided positive and negative regression tests
Copy link
Collaborator

@antoniivanov antoniivanov left a comment

Choose a reason for hiding this comment

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

Looks good to me. I made some comments.

@antoniivanov
Copy link
Collaborator

I don't know if you've noticed but there are some failing tests: https://gitlab.com/vmware-analytics/versatile-data-kit/-/jobs/3552673519

Have you be able to run them locally ? In IDE ?

@antoniivanov antoniivanov changed the title vdk-impala/vdk-plugins: Introduce checks for scd1 template vdk-impala: Introduce checks for scd1 template Jan 5, 2023
@sbuldeev
Copy link
Collaborator Author

sbuldeev commented Jan 5, 2023

I don't know if you've noticed but there are some failing tests: https://gitlab.com/vmware-analytics/versatile-data-kit/-/jobs/3552673519

Have you be able to run them locally ? In IDE ?

These tests are failing, because at some point the execute_tempalate behavior is trying to dump all the arguments and since there is a function passed as an argument this is not possible.

Copy link
Collaborator

@antoniivanov antoniivanov left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Thanks for the hard work on this change.

@antoniivanov
Copy link
Collaborator

antoniivanov commented Jan 23, 2023

@sbuldeev, before merging do ping me so I can walk you through the proper process for merging a change.

A) It's fairly important that there's one commit (in this case particularly) - so you need to use Squash option
B) It's important that the commit description is the same as the PR description.
There also should be Signed-off-by: Name mail@example.com in the commit description.

Copy link
Collaborator

@murphp15 murphp15 left a comment

Choose a reason for hiding this comment

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

Looks good

@sbuldeev sbuldeev merged commit fe24b48 into main Jan 23, 2023
@sbuldeev sbuldeev deleted the person/sbuldeev/add-quality-check-to-processing-templates branch January 23, 2023 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants