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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 14 commits into from
Jun 22, 2021
Merged

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

Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2c6d916
feature to support Looker self-hosted instance domain(#3393)
May 25, 2021
78cd567
Merge branch 'master' of https://github.com/airbytehq/airbyte into so…
May 25, 2021
da173ab
changing self-hosted instances to any url(#3393)
May 26, 2021
846c10d
Merge branch 'master' of https://github.com/airbytehq/airbyte into so…
May 26, 2021
6af8b3d
fixing build failures
May 27, 2021
2782739
regexp for any URL
May 31, 2021
5be65ce
fixing build failures
May 31, 2021
8c6f412
fixing regexp pattern and adding unittests
Jun 3, 2021
7d58efa
writing unitests using pytest
Jun 3, 2021
a245335
pytest-fixture for unit-tests
Jun 3, 2021
4ff70eb
Merge branch 'master' into source-looker
vitaliizazmic Jun 11, 2021
cd9e005
Source Looker #3587 - remove domain spec field pattern
vitaliizazmic Jun 14, 2021
319b73a
Source Looker #3587 - fix description of domain spec field
vitaliizazmic Jun 17, 2021
a651528
Source Looker #3393 - bum version
vitaliizazmic Jun 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions airbyte-integrations/connectors/source-looker/source_looker/spec.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,15 @@
"properties": {
"domain": {
"type": "string",
"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$)|(^(?:[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})?$)"
Copy link
Contributor

@sherifnada sherifnada Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Janardhanpoola Janardhanpoola Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

(^(?:[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:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

agree, please add a unit test for this.

],
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

"examples": [
"domainname.looker.com",
"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"
Copy link
Contributor

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

Choose a reason for hiding this comment

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

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"

},
"client_id": {
"title": "Client ID",
Expand Down