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: fix impala template empty source view usr err #1189

Merged
merged 4 commits into from
Sep 27, 2022

Conversation

mrMoZ1
Copy link
Contributor

@mrMoZ1 mrMoZ1 commented Sep 23, 2022

what: The template arguments validator now makes use of the skip_remaining_steps() method when an empty source view is returned.

why: Users of the template requested that empty source view stopped resulting in a User Error, after a discussion it was agreed that this is the correct and desired behaviour.

testing: this change depends on #1188
Edited failing regression tests.

Signed-off-by: Momchil Zhivkov mzhivkov@vmware.com

Signed-off-by: mrMoZ1 <mzhivkov@vmware.com>
@mrMoZ1 mrMoZ1 force-pushed the person/mzhivkov/fix-impala-template-regression branch from e9cfbb7 to 78f95ea Compare September 26, 2022 15:41
@antoniivanov
Copy link
Collaborator

I will never tire asking this :) Tests ?

Any bug we fix should have tests. I am surprised that no tests fail (clearly we have not added tests covering this scenario - what happens if SQL view returns 0 rows) but now it's a good opportunity to do so

@mrMoZ1
Copy link
Contributor Author

mrMoZ1 commented Sep 26, 2022

I will never tire asking this :) Tests ?

Any bug we fix should have tests. I am surprised that no tests fail (clearly we have not added tests covering this scenario - what happens if SQL view returns 0 rows) but now it's a good opportunity to do so

Tests will likely fail. And I will update the MR accordingly.

Signed-off-by: mrMoZ1 <mzhivkov@vmware.com>
@antoniivanov
Copy link
Collaborator

Some small comment around test verification but looks good to me. Thanks.

@mrMoZ1 mrMoZ1 enabled auto-merge (squash) September 27, 2022 12:33
@mrMoZ1 mrMoZ1 merged commit 6c6a8bc into main Sep 27, 2022
@mrMoZ1 mrMoZ1 deleted the person/mzhivkov/fix-impala-template-regression branch September 27, 2022 12:45
assert "Source view returned no results." in res.exception.args[0]
assert isinstance(res.exception, errors.UserCodeError)
assert not res.exception
assert res.exit_code == 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know it's merged but I am leaving the comment for future reference (we can leave it now this way) :

There's util cli_assert (https://github.com/vmware/versatile-data-kit/blob/main/projects/vdk-plugins/vdk-test-utils/src/vdk/plugin/test_utils/util_funcs.py#L53) utility that should be used, It prints a lot of more details about the execution if the assert fails.

antoniivanov pushed a commit that referenced this pull request Sep 29, 2022
what: The template arguments validator now makes use of the skip_remaining_steps() method when an empty source view is returned.

why: Users of the template requested that empty source view stopped resulting in a User Error, after a discussion it was agreed that this is the correct and desired behaviour.

testing: this change depends on #1188
Edited failing regression tests.

Signed-off-by: Momchil Zhivkov mzhivkov@vmware.com
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

3 participants