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

🎉 New Source: Okta #3563

Merged
merged 16 commits into from
Jun 2, 2021
Merged

🎉 New Source: Okta #3563

merged 16 commits into from
Jun 2, 2021

Conversation

grebessi
Copy link
Contributor

@grebessi grebessi commented May 24, 2021 •
edited by sync-by-unito bot
Loading

What

Added new source to Okta.
This PR closes #2414

How

Added support for the following schemas:

  • Users
  • Groups
  • System Log

Pre-merge Checklist

  • Run integration tests
  • Publish Docker images

┆Issue is synchronized with this Asana task by Unito

@auto-assign auto-assign bot requested a review from sherifnada May 24, 2021 08:18
@sherifnada sherifnada removed the request for review from jrhizor May 24, 2021 18:31
keu
keu suggested changes May 24, 2021
View reviewed changes
Copy link
Contributor

@keu keu left a comment

Choose a reason for hiding this comment

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

minor changes

sherifnada
sherifnada approved these changes May 26, 2021
View reviewed changes
Copy link
Contributor

@sherifnada sherifnada left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Very small comments. Please bump and publish once you've addressed them.

Also, please change the PR name to 🎉 New Source: Okta

airbyte-integrations/connectors/source-okta/sample_files/configure_catalog.json Outdated
@@ -0,0 +1,13 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems not to belong to this connector

airbyte-integrations/connectors/source-okta/source_okta/source.py Outdated
self._url_base = url_base

@property
def page_size(self) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this not a hardcoded property e.g: page_size = 200?

airbyte-integrations/connectors/source-okta/sample_files/catalog.json
"name": "logs",
"json_schema": {}
},
"sync_mode": "incremental",
Copy link
Contributor

Choose a reason for hiding this comment

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

this catalog used for testing should contain the cursor field and primary key for each stream

airbyte-integrations/connectors/source-okta/integration_tests/catalog.json
"name": "groups",
"json_schema": {}
},
"sync_mode": "incremental",
Copy link
Contributor

Choose a reason for hiding this comment

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

this catalog should contain PKs and cursor fields

sherifnada
sherifnada suggested changes May 26, 2021
View reviewed changes
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

Actually just realized a few things:

  1. Which API token are you using? I thought Okta was an Oauth based source
  2. Could you add the config to Github secrets, ci_credentials.sh, test-command and publish-command.yml?
  3. we need to add docs for this connector in the docs/ directory. Could you pattern match off the existing connectors to do this? Also, we need to update SUMMARY.md to point to these docs.

@grebessi grebessi changed the title Source: Okta New Source: Okta May 26, 2021
@grebessi
Copy link
Contributor Author

Actually just realized a few things:

  1. Which API token are you using? I thought Okta was an Oauth based source
  2. Could you add the config to Github secrets, ci_credentials.sh, test-command and publish-command.yml?
  3. we need to add docs for this connector in the docs/ directory. Could you pattern match off the existing connectors to do this? Also, we need to update SUMMARY.md to point to these docs.
  1. According to the docs, we can authenticate in 5 ways:
  • Public Application
  • Trusted Application
  • Activation Token
  • Device Fingerprinting
  • Password Expiration Warning
    In this case, I did use Trusted Application. I used the token provided by LastPass (Create the token docs)

2 and 3, I will start working on adding it.

@grebessi grebessi requested a review from sherifnada June 1, 2021 15:56
sherifnada
sherifnada approved these changes Jun 1, 2021
View reviewed changes
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

LGTM -- please publish and merge

docs/integrations/README.md Outdated Show resolved Hide resolved
@grebessi
Copy link
Contributor Author

grebessi commented Jun 1, 2021 •
edited
Loading

/publish connector=connectors/source-okta

@grebessi
Copy link
Contributor Author

grebessi commented Jun 1, 2021 •
edited
Loading

/publish connector=connectors/source-okta

🕑 connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/897149330
❌ connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/897149330

@grebessi
Copy link
Contributor Author

grebessi commented Jun 1, 2021 •
edited
Loading

/publish connector=connectors/source-okta

🕑 connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/897203628
❌ connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/897203628

@grebessi
Copy link
Contributor Author

grebessi commented Jun 2, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/source-okta

🕑 connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/899887311
❌ connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/899887311

@grebessi
Copy link
Contributor Author

grebessi commented Jun 2, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/source-okta

🕑 connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/900130335
✅ connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/900130335

@grebessi grebessi merged commit eade88e into master Jun 2, 2021
@grebessi grebessi deleted the grebessi/source-okta branch June 2, 2021 17:03
@avaidyanatha avaidyanatha changed the title New Source: Okta 🎉 New Source: Okta Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@po3na4skld po3na4skld po3na4skld approved these changes

@sherifnada sherifnada sherifnada approved these changes

@keu keu keu approved these changes

@davinchia davinchia Awaiting requested review from davinchia

@htrueman htrueman Awaiting requested review from htrueman

@Zirochkaa Zirochkaa Awaiting requested review from Zirochkaa

@vitaliizazmic vitaliizazmic Awaiting requested review from vitaliizazmic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Source: Okta
4 participants
@grebessi @keu @sherifnada @po3na4skld