Fix LookerHook serialize missing 1 argument error#34678
Merged
hussein-awala merged 5 commits intoapache:mainfrom Oct 2, 2023
Merged
Fix LookerHook serialize missing 1 argument error#34678hussein-awala merged 5 commits intoapache:mainfrom
hussein-awala merged 5 commits intoapache:mainfrom
Conversation
Contributor
|
@cjonesy Would it make sense to add a testcase to make sure you are no longer facing the issue |
Contributor
Author
@utkarsharma2 - Good catch! Tests have been added. |
RNHTTR
approved these changes
Oct 2, 2023
utkarsharma2
approved these changes
Oct 2, 2023
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
Member
|
Congrats on your first commit 🎉 |
98 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a ticket open for this back in March (#30169), it was closed out due to inactivity and there was a comment that it was related to an upstream issue in
looker_sdk. I took a look at the issue inlooker_sdkand it didn't quite seem like the same thing.After digging in, I believe this is simply a typo in the hook's
get_looker_sdk()method and the40was left off by mistake. Adding the40back on here fixes the issues in my local environment. I can't seem to find any reason it would be desirable to useserialize.serializethere, and in fact looker_sdk usesserialize40here.You can see here in the
looker_sdkrepo that during init we useserialize.serialize40, whereas inLookerHookwe useserialize.serialize.serialize.serializepoints here, where it expects aconverterto be passed in.serialize.serialize40points here, where there is a defaultconverterset.This becomes problematic when you want to do something with the sdk from
LookerHookwhen calling a method usingpost, likecreate_query.An example - when we try to do something like this, we get an error due to the missing
converter:This generates the error:
TypeError: serialize() missing 1 required keyword-only argument: 'converter'^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.