🎉 Source Looker: enable supporting of customer-hosted (self-hosted) instances #3587

Janardhanpoola · 2021-05-25T14:41:53Z

What

Adding support for self/customer hosted looker instance.(#3393)

How

Describe the solution

Pre-merge Checklist

Run integration tests
Publish Docker images

Recommended reading order

test.java
component.ts
the rest

…urce-looker

sherifnada · 2021-05-25T14:45:53Z

airbyte-integrations/connectors/source-looker/source_looker/spec.json

-        "pattern": ["^[a-zA-Z0-9._-]*\\.looker\\.com$"],
-        "examples": ["domainname.looker.com"],
-        "description": "Domain for your Looker account, e.g. airbyte.cloud.looker.com"
+        "pattern": ["(^[a-zA-Z0-9._-]*\\.looker\\.com$)|(^looker\\.[a-zA-Z0-9._-]*\\.com$)|(^[0-9]+(?:\\.[0-9]+){3}(:[0-9]+)?$)"],


Why should the second pattern have looker in the name? It should just be any url right?

I see looker.[yourdomain].com in the documentation.
https://docs.looker.com/setup-and-management/on-prem-install/installation#create_a_dns_record

If that's not the case , ill change to support any url

I'm guessing that's more of a suggestion than a requirement. We can probably give that pattern in the examples array but not strictly require it.

Also we shouldn't require .com

could you give me some examples so that i get clarity on how to write regex pattern

…urce-looker

vitaliizazmic · 2021-05-28T16:10:32Z

airbyte-integrations/connectors/source-looker/source_looker/spec.json

-        "description": "Domain for your Looker account, e.g. airbyte.cloud.looker.com"
+        "pattern": [
+          "(^[a-zA-Z0-9._-]*\\.looker\\.com$)|(^[a-zA-Z0-9._-]*\\.[a-zA-Z0-9._-]*\\.[a-zA-Z0-9._-]*$)|(^[0-9]+(?:\\.[0-9]+){3}(:[0-9]+)?$)"
+        ],


The pattern needs to be checked. For example, the second group requires entry string dived at least by two dots. I think this is wrong. The third group has no segment length limitation.

sherifnada · 2021-06-01T21:56:28Z

airbyte-integrations/connectors/source-looker/source_looker/spec.json

-        "examples": ["domainname.looker.com"],
-        "description": "Domain for your Looker account, e.g. airbyte.cloud.looker.com"
+        "pattern": [
+          "(^[a-zA-Z0-9._-]*\\.looker\\.com$)|(^(?:[a-zA-Z0-9._-]*)?[a-zA-Z0-9._-]*\\.[a-zA-Z0-9._-]*$)|(^\\d{1,3}\\.\\d{1,3}\\.\\d{1,3}\\.\\d{1,3}(?::\\d{1,5})?$)"


can you create unit tests to verify this works? I'm not sure the second regex works as intended. I tested it on https://regex101.com/ with looker.airbyte.com and it didn't work.

If it's too hard to pin down the regex then we can remove the pattern entirely and rely on the check operation to verify the domain is correct.

(^(?:[a-zA-Z0-9._-]*)?[a-zA-Z0-9._-]*\\.[a-zA-Z0-9._-]*$)-->pattern in spec.json

(^(?:[a-zA-Z0-9._-]*)?[a-zA-Z0-9._-]*\.[a-zA-Z0-9._-]*$)--->pattern in python(used for testing in regexp101)

Did you try changing \\. to \. and then test it on regexp101. since '.' have special meaning ..we use \ as an escape character in python. But regexp pattern in spec.json file expects us to use \\ as an escape character. Same applies for matching the digits \d in third pattern. Please find the snap below that matches several domains and let me know if any changes required:

@Janardhanpoola hm, what about my.shiny.website.com:1234?

agree, please add a unit test for this.

keu · 2021-06-03T13:32:59Z

airbyte-integrations/connectors/source-looker/unit_tests/test_URL.py

+import re
+import unittest
+
+def valid_URL(test_str):


do you know how we can avoid duplication and use regex from spec file here?

I think may be we can access spec dict and can refer pattern using spec["properties"]["domain"]["pattern"]. I'm quite unsure of the imports here. can you point me an example here

try to search @pytest.fixture in our codebase, there will be plenty of examples

Added @pytest.fixture as suggested

keu · 2021-06-03T13:34:51Z

airbyte-integrations/connectors/source-looker/unit_tests/test_URL.py

+
+
+
+class TestURLPattern(unittest.TestCase):


also we are trying to follow pytest-style tests, please check this for example how you can do it

keu

see comments

vitaliizazmic · 2021-06-03T14:38:23Z

airbyte-integrations/connectors/source-looker/unit_tests/test_URL.py

@@ -0,0 +1,44 @@
+import re


Naming shall conform to PEP8 Naming Conventions.

changed filename and tests as per pep8 standards

keu · 2021-06-03T20:44:34Z

airbyte-integrations/connectors/source-looker/unit_tests/test_url.py

+        return result.group(0)
+
+@pytest.fixture
+def url():


this is not what I meant, there is an example of parameterized tests on the page that I sent you.
You don't need to repeat the same test again and again

keu · 2021-06-03T20:46:13Z

airbyte-integrations/connectors/source-looker/unit_tests/test_url.py

+import re
+import pytest
+
+def valid_url(test_str):


also, this still does not prevents us from duplication. What will happen with this test if we decide to change the spec?

you can check

airbyte/airbyte-integrations/connectors/source-freshdesk/integration_tests/test_client.py

Line 36 in e602133

@pytest.fixture(scope="session")

for examples

yevhenii-ldv · 2021-06-15T09:55:26Z

airbyte-integrations/connectors/source-looker/source_looker/spec.json

+          "looker.clientname.com",
+          "123.123.124.123:8000"
+        ],
+        "description": "Domain for your Looker account, e.g. airbyte.cloud.looker.com,looker.[clientname].com,ip address"


Suggested change

"description": "Domain for your Looker account, e.g. airbyte.cloud.looker.com,looker.[clientname].com,ip address"

"description": "Domain for your Looker account, e.g. airbyte.cloud.looker.com, looker.[clientname].com, IP address"

vitaliizazmic · 2021-06-17T07:13:39Z

/test connector=source-looker

🕑 source-looker https://github.com/airbytehq/airbyte/actions/runs/945518267
✅ source-looker https://github.com/airbytehq/airbyte/actions/runs/945518267

sherifnada · 2021-06-22T06:23:14Z

@vitaliizazmic make sure to format the PR name correctly

vitaliizazmic · 2021-06-22T11:43:36Z

/publish connector=connectors/source-looker

🕑 connectors/source-looker https://github.com/airbytehq/airbyte/actions/runs/960496864
✅ connectors/source-looker https://github.com/airbytehq/airbyte/actions/runs/960496864

root added 2 commits May 25, 2021 19:19

feature to support Looker self-hosted instance domain(#3393)

2c6d916

Merge branch 'master' of https://github.com/airbytehq/airbyte into so…

78cd567

…urce-looker

Janardhanpoola assigned vitaliizazmic May 25, 2021

auto-assign bot requested review from masonwheeler and sherifnada May 25, 2021 14:41

sherifnada suggested changes May 25, 2021

View reviewed changes

root added 3 commits May 26, 2021 17:49

changing self-hosted instances to any url(#3393)

da173ab

Merge branch 'master' of https://github.com/airbytehq/airbyte into so…

846c10d

…urce-looker

fixing build failures

6af8b3d

vitaliizazmic linked an issue May 28, 2021 that may be closed by this pull request

Add a feature to support Looker self-hosted instance domain #3393

Closed

vitaliizazmic requested review from vitaliizazmic and removed request for masonwheeler May 28, 2021 15:32

vitaliizazmic assigned Janardhanpoola and unassigned vitaliizazmic May 28, 2021

vitaliizazmic suggested changes May 28, 2021

View reviewed changes

root added 2 commits May 31, 2021 23:19

regexp for any URL

2782739

fixing build failures

5be65ce

vitaliizazmic approved these changes Jun 1, 2021

View reviewed changes

vitaliizazmic requested review from htrueman, po3na4skld, vovavovavovavova, yevhenii-ldv and Zirochkaa June 1, 2021 14:21

sherifnada suggested changes Jun 1, 2021

View reviewed changes

fixing regexp pattern and adding unittests

8c6f412

keu reviewed Jun 3, 2021

View reviewed changes

keu requested changes Jun 3, 2021

View reviewed changes

writing unitests using pytest

7d58efa

vitaliizazmic suggested changes Jun 3, 2021

View reviewed changes

pytest-fixture for unit-tests

a245335

keu reviewed Jun 3, 2021

View reviewed changes

keu changed the title ~~Source looker~~ Source Looker: support self-hosted domains Jun 7, 2021

sherifnada unassigned Janardhanpoola Jun 10, 2021

vitaliizazmic added 2 commits June 11, 2021 16:31

Merge branch 'master' into source-looker

4ff70eb

Source Looker #3587 - remove domain spec field pattern

cd9e005

vitaliizazmic requested a review from keu June 14, 2021 13:32

yevhenii-ldv reviewed Jun 15, 2021

View reviewed changes

yevhenii-ldv approved these changes Jun 15, 2021

View reviewed changes

keu approved these changes Jun 15, 2021

View reviewed changes

vitaliizazmic approved these changes Jun 17, 2021

View reviewed changes

Source Looker #3587 - fix description of domain spec field

319b73a

github-actions bot added the area/connectors Connector related issues label Jun 17, 2021

vitaliizazmic requested a review from sherifnada June 17, 2021 08:06

sherifnada approved these changes Jun 22, 2021

View reviewed changes

vitaliizazmic changed the title ~~Source Looker: support self-hosted domains~~ 🎉 Source Looker: enable supporting of customer-hosted (self-hosted) instances Jun 22, 2021

Source Looker #3393 - bum version

a651528

vitaliizazmic merged commit 3d44999 into master Jun 22, 2021

vitaliizazmic deleted the source-looker branch June 22, 2021 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉 Source Looker: enable supporting of customer-hosted (self-hosted) instances #3587

🎉 Source Looker: enable supporting of customer-hosted (self-hosted) instances #3587

Janardhanpoola commented May 25, 2021

sherifnada May 25, 2021

Janardhanpoola May 26, 2021

sherifnada May 26, 2021

sherifnada May 26, 2021

Janardhanpoola May 26, 2021

vitaliizazmic May 28, 2021

sherifnada Jun 1, 2021 •

edited

Loading

Janardhanpoola Jun 2, 2021 •

edited

Loading

keu Jun 3, 2021

keu Jun 3, 2021

keu Jun 3, 2021 •

edited

Loading

Janardhanpoola Jun 3, 2021

keu Jun 3, 2021

Janardhanpoola Jun 3, 2021 •

edited

Loading

keu Jun 3, 2021

keu left a comment

vitaliizazmic Jun 3, 2021

Janardhanpoola Jun 3, 2021

keu Jun 3, 2021 •

edited

Loading

keu Jun 3, 2021

keu Jun 3, 2021 •

edited

Loading

yevhenii-ldv Jun 15, 2021 •

edited by keu

Loading

vitaliizazmic commented Jun 17, 2021 •

edited by github-actions bot

Loading

sherifnada commented Jun 22, 2021

vitaliizazmic commented Jun 22, 2021 •

edited by github-actions bot

Loading

	"description": "Domain for your Looker account, e.g. airbyte.cloud.looker.com,looker.[clientname].com,ip address"
	"description": "Domain for your Looker account, e.g. airbyte.cloud.looker.com, looker.[clientname].com, IP address"




		class TestURLPattern(unittest.TestCase):

🎉 Source Looker: enable supporting of customer-hosted (self-hosted) instances #3587

🎉 Source Looker: enable supporting of customer-hosted (self-hosted) instances #3587

Conversation

Janardhanpoola commented May 25, 2021

What

How

Pre-merge Checklist

Recommended reading order

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherifnada Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

Janardhanpoola Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keu Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janardhanpoola Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keu Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keu Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

yevhenii-ldv Jun 15, 2021 • edited by keu Loading

Choose a reason for hiding this comment

vitaliizazmic commented Jun 17, 2021 • edited by github-actions bot Loading

sherifnada commented Jun 22, 2021

vitaliizazmic commented Jun 22, 2021 • edited by github-actions bot Loading

sherifnada Jun 1, 2021 •

edited

Loading

Janardhanpoola Jun 2, 2021 •

edited

Loading

keu Jun 3, 2021 •

edited

Loading

Janardhanpoola Jun 3, 2021 •

edited

Loading

keu Jun 3, 2021 •

edited

Loading

keu Jun 3, 2021 •

edited

Loading

yevhenii-ldv Jun 15, 2021 •

edited by keu

Loading

vitaliizazmic commented Jun 17, 2021 •

edited by github-actions bot

Loading

vitaliizazmic commented Jun 22, 2021 •

edited by github-actions bot

Loading