-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃帀 Source Looker: add run-look endpoint #3911
Conversation
@mjirv LMK if there is anything we can help with! |
This reverts commit 7ecaaa4.
Hi @sherifnada, I think it's ready for review now. Any comments you all have would be appreciated, but the one thing I'm curious about is if you have any recommendations about what kind of tests to add since there aren't any on the connector yet. |
@mjirv Sherif is on PTO now. I will get back to you by tomorrow |
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.
@mjirv This logic seems sound to me.
Regarding tests, we do have integration tests they aren't easily modified by contributors. I'll create a follow up ticket and we'll take care of it. I'd like to have test on the the dynamic conversion logic - the _get_run_look_json_schema
function and logic within - before we merge this in since that seems to be the main complexity here. Feel free to use unit_test.py
.
Can you also take a look at the connector checklist? The documentation section should be the most relevant.
Can you also run the integration test locally and paste a screenshot of the successful output? Basic sanity check this is working. You'll have to place the credentials in secrets/config.json
.
Thanks!
airbyte-integrations/connectors/source-looker/sample_files/full_configured_catalog.json
Show resolved
Hide resolved
airbyte-integrations/connectors/source-looker/source_looker/client.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-looker/source_looker/client.py
Outdated
Show resolved
Hide resolved
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.
Could you also update the documentation in source/looker.md
to include this new stream?
airbyte-integrations/connectors/source-looker/sample_files/full_configured_catalog.json
Show resolved
Hide resolved
Thanks @davinchia and @tuliren. I've made updates for all your comments aside from adding unit tests (& still have to go through the connector checklist in more detail). |
@mjirv, I guess you are probably busy with other things. It's totally fine. I will merge this PR as is in the next hour, and take care of the remaining items on the checklist in a follow up PR. Feel free to open another PR if there is anything else you want to add. |
/test connector=connectors/source-looker
|
Thank you @tuliren! Yeah, I got busy here the last couple weeks and didn't have a chance to get back to it yet. I appreciate you moving it forward! |
Sure, no problem. Thank you for putting together this PR. |
What
This PR adds a Run Look output stream to the Looker connector. Addresses #3649.
It works but isn't totally ready to go yet. I want to work on the following:
How
Implements a
stream__run_looks
method inclient.py
along with methods to generate a json_schema from the provided look(s). Also adds a new configuration field for Look IDs to run.Recommended reading order
spec.json
client.py
Pre-merge Checklist
Expand the checklist which is relevant for this PR.
Connector checklist
airbyte_secret
in output spec./gradlew :airbyte-integrations:connectors:<name>:integrationTest
./test connector=connectors/<name>
command as documented here is passing.docs/integrations/
directory./publish
command described here