🐛 Source Okta: Fix Infinite Loop in GroupMembers Stream #20833

michaelcabiareyes · 2022-12-22T18:19:55Z

What

Resolves #20812

How

Reset the next_page_token for every group from stream slice and apply next_page_token for continuation of fetching.

🚨 User Impact 🚨

N/A

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

============================================================================================ short test summary info ============================================================================================= SKIPPED [1] ..\..\bases\source-acceptance-test\source_acceptance_test\tests\test_core.py:94: The previous and actual specifications are identical.
FAILED ..\..\bases\source-acceptance-test\source_acceptance_test\tests\test_core.py::TestDiscovery::test_backward_compatibility[inputs0] - source_acceptance_test.utils.backward_compatibility.NonBackwardCompat...FAILED ..\..\bases\source-acceptance-test\source_acceptance_test\tests\test_core.py::TestBasicRead::test_read[inputs0] - Failed: Stream groups: All expected records must be produced

Results (120.20s):
      28 passed
       2 failed
         - airbyte-integrations\bases\source-acceptance-test\source_acceptance_test\tests/test_core.py:461 TestDiscovery.test_backward_compatibility[inputs0]
         - airbyte-integrations\bases\source-acceptance-test\source_acceptance_test\tests/test_core.py:660 TestBasicRead.test_read[inputs0]
       1 skipped

CLAassistant · 2022-12-22T18:20:00Z

All committers have signed the CLA.

koconder · 2022-12-28T00:50:34Z

/test connector=connectors/source-okta

🕑 connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/3790860759
❌ connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/3790860759
🐛 https://gradle.com/s/m5oxx23tfqkx4

Build Failed

Test summary info:

	 =========================== short test summary info ============================
	 FAILED unit_tests/test_streams.py::TestStreamGroupMembers::test_group_members_request_params_with_latest_entry
	 �[31m================== �[31m�[1m1 failed�[0m, �[32m59 passed�[0m, �[33m142 warnings�[0m�[31m in 1.12s�[0m�[31m ==================�[0m

koconder · 2023-01-01T09:47:35Z

@michaelcabiareyes Hey, I'm one of the Airbyte maintainers and I'll be taking a look at your PR. Firstly thanks for your contribution :) The code logic seems sounds but the unit tests are failing, see the results in this PR. Could you please take a look at the unit test results and see if you can resolve. Reach out if you have any questions.

michaelcabiareyes · 2023-01-02T06:54:02Z

Hi @koconder. Thank you for your response! Sure thing, let me take a look at the unit test and try to resolve this.

natalyjazzviolin · 2023-01-18T14:17:44Z

@michaelcabiareyes do you have any updates?

michaelcabiareyes · 2023-01-21T18:22:40Z

@michaelcabiareyes do you have any updates?

Hi @natalyjazzviolin . Yes, I'm currently close in finishing the resolution of the failed unit test and would be able to update 2-3 days from now.

michaelcabiareyes · 2023-01-22T03:51:45Z

/test connector=connectors/source-okta

koconder · 2023-01-22T04:12:40Z

@michaelcabiareyes ill need to run the unit tests using github actions. Its not able to be triggered by public.

koconder · 2023-01-22T04:12:45Z

/test connector=connectors/source-okta

🕑 connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/3978035263
❌ connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/3978035263
🐛 https://gradle.com/s/v6dxvkjhfo3va

Build Failed

Test summary info:

	 =========================== short test summary info ============================
	 FAILED unit_tests/test_streams.py::TestStreamGroupMembers::test_group_members_request_params_with_latest_entry
	 �[31m================== �[31m�[1m1 failed�[0m, �[32m59 passed�[0m, �[33m142 warnings�[0m�[31m in 1.14s�[0m�[31m ==================�[0m

michaelcabiareyes · 2023-01-22T04:28:32Z

@michaelcabiareyes ill need to run the unit tests using github actions. Its not able to be triggered by public.

Oh, I see. Thank you @koconder!

koconder · 2023-01-22T05:01:01Z

@michaelcabiareyes fyi test is still failing. You can follow the steps in the connector readme to use gradle commands to test locally the acceptance tests before you make your changes to verify 100%

michaelcabiareyes · 2023-01-22T11:36:34Z

@michaelcabiareyes fyi test is still failing. You can follow the steps in the connector readme to use gradle commands to test locally the acceptance tests before you make your changes to verify 100%

Understood, I think I have pushed the changes in the master branch instead of the okta-group-members-stream. I'll try again now.

…en param

michaelcabiareyes

I reset the params variable instead of inheriting the params from the parent class as the variables inside these are not required in the query [GroupMembers API]. Along with this, I've also removed the next_cursor in the unit testing.

koconder · 2023-01-23T01:24:04Z

/test connector=connectors/source-okta

🕑 connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/3982519582
❌ connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/3982519582
🐛 https://gradle.com/s/q4xnw4ph2c4jg

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestBasicRead::test_read[inputs0] - Failed: Stream users...
FAILED test_incremental.py::TestIncremental::test_two_sequential_reads[inputs0]
FAILED test_incremental.py::TestIncremental::test_read_sequential_slices[inputs0]
FAILED test_incremental.py::TestIncremental::test_state_with_abnormally_large_values[inputs0]
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:94: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:377: The previous and actual discovered catalogs are identical.
============= 4 failed, 27 passed, 2 skipped in 145.58s (0:02:25) ==============

michaelcabiareyes · 2023-01-23T04:32:47Z

Hi @koconder , I am trying to figure out how to resolve the failed acceptance tests. Can you help me point out where should I specifically resolve it and how? Because I see that it failed the users stream even though I've only modified the GroupMembers stream.

koconder · 2023-01-23T10:59:16Z

@natalyjazzviolin can you give any guidance on the unit testing side of things?

michaelcabiareyes · 2023-01-29T12:58:37Z

Hello @koconder. Can you run the unit test again? So I could check if the changes I've made is correct. Thank you!

koconder · 2023-01-29T23:30:25Z

/test connector=connectors/source-okta

🕑 connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/4039183208
❌ connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/4039183208
🐛 https://gradle.com/s/t5f6tbtnvjjcy

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestBasicRead::test_read[inputs0] - Failed: Stream users...
FAILED test_incremental.py::TestIncremental::test_two_sequential_reads[inputs0]
FAILED test_incremental.py::TestIncremental::test_read_sequential_slices[inputs0]
FAILED test_incremental.py::TestIncremental::test_state_with_abnormally_large_values[inputs0]
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:94: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:377: The previous and actual discovered catalogs are identical.
============= 4 failed, 27 passed, 2 skipped in 150.30s (0:02:30) ==============

michaelcabiareyes · 2023-01-30T09:44:52Z

/test connector=connectors/source-okta

🕑 connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/4039183208
❌ connectors/source-okta https://github.com/airbytehq/airbyte/actions/runs/4039183208
🐛 https://gradle.com/s/t5f6tbtnvjjcy

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestBasicRead::test_read[inputs0] - Failed: Stream users...
FAILED test_incremental.py::TestIncremental::test_two_sequential_reads[inputs0]
FAILED test_incremental.py::TestIncremental::test_read_sequential_slices[inputs0]
FAILED test_incremental.py::TestIncremental::test_state_with_abnormally_large_values[inputs0]
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:94: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:377: The previous and actual discovered catalogs are identical.
============= 4 failed, 27 passed, 2 skipped in 150.30s (0:02:30) ==============

Still failed the testing. I am trying to replicate but I am not able to. Can you help me figure it out @natalyjazzviolin? Because I don't think that incremental tests should be done in GroupMembers stream as this resets per group iteration.

michaelcabiareyes · 2023-01-31T03:24:52Z

============================================================================================ short test summary info ============================================================================================= SKIPPED [1] ..\..\bases\source-acceptance-test\source_acceptance_test\tests\test_core.py:94: The previous and actual specifications are identical.
FAILED ..\..\bases\source-acceptance-test\source_acceptance_test\tests\test_core.py::TestDiscovery::test_backward_compatibility[inputs0] - source_acceptance_test.utils.backward_compatibility.NonBackwardCompat...FAILED ..\..\bases\source-acceptance-test\source_acceptance_test\tests\test_core.py::TestBasicRead::test_read[inputs0] - Failed: Stream groups: All expected records must be produced

Results (120.20s):
      28 passed
       2 failed
         - airbyte-integrations\bases\source-acceptance-test\source_acceptance_test\tests/test_core.py:461 TestDiscovery.test_backward_compatibility[inputs0]
         - airbyte-integrations\bases\source-acceptance-test\source_acceptance_test\tests/test_core.py:660 TestBasicRead.test_read[inputs0]
       1 skipped

I had this test summary when I tried disabling other streams except for Groups and GroupMembers. I suspect that due to the sample data being limited to one group only, it is not able to reproduce the same result I am having. I have also disabled the Incremental in GroupMembers as this is not applicable due to the need to reset the group_id for every stream slice.

sh4sh · 2023-02-09T17:11:13Z

Hey @michaelcabiareyes, thanks for your work on this PR!

My understanding is that it's possible a user may have only one group configured. Can you ensure that this case is accounted for?

michaelcabiareyes · 2023-02-10T03:53:29Z

Hey @michaelcabiareyes, thanks for your work on this PR!

My understanding is that it's possible a user may have only one group configured. Can you ensure that this case is accounted for?

Hi @sh4sh. Thank you for your response! Yes, a user may belong to one or more groups. Can you confirm if the test account used for this connector has only one group and all users under the account belonged to that group? This is what I understood on the TestBasicRead.test_read[inputs0] as this checks if the records is the same with the contents of "integrationt_tests/expected_records.jsonl".

archangelic · 2023-06-28T23:27:06Z

I'm currently reviewing this, but there are some merge conflicts that need to be resolved before we continue.

marcosmarxm

Thanks for the contribution @michaelcabiareyes

Source Okta: Fix Unlimited Looping in GroupMembers Stream

c88104d

octavia-squidington-iv added area/connectors Connector related issues bounty community connectors/source/okta labels Dec 22, 2022

michaelcabiareyes changed the title ~~Source Okta: Fix Unlimited Looping in GroupMembers Stream~~ Source Okta: Fix Infinite Loop in GroupMembers Stream Dec 22, 2022

Merge branch 'master' into okta-group-members-stream

b9e598e

sajarin added bounty-M Maintainer program: claimable medium bounty PR bounty-S Maintainer program: claimable small bounty PR and removed bounty-M Maintainer program: claimable medium bounty PR labels Jan 2, 2023

sajarin assigned koconder Jan 2, 2023

Merge branch 'master' into okta-group-members-stream

6af4cdc

Source Okta: Resolve unit test error through removal of next_page_tok…

963688c

…en param

michaelcabiareyes commented Jan 22, 2023

View reviewed changes

Merge branch 'master' into okta-group-members-stream

41813db

Merge branch 'master' into okta-group-members-stream

962cd8b

michaelcabiareyes changed the title ~~Source Okta: Fix Infinite Loop in GroupMembers Stream~~ 🐛 Source Okta: Fix Infinite Loop in GroupMembers Stream Jan 31, 2023

Merge branch 'master' into okta-group-members-stream

f326abf

michaelcabiareyes requested a review from natalyjazzviolin March 9, 2023 19:05

marcosmarxm unassigned koconder Apr 7, 2023

marcosmarxm added the pr-alpha-beta label Apr 14, 2023

marcosmarxm assigned archangelic Apr 14, 2023

Merge branch 'master' into okta-group-members-stream

f306da2

marcosmarxm unassigned archangelic May 12, 2023

marcosmarxm added community-legacy and removed community labels May 15, 2023

jrolom removed the bounty label Jun 9, 2023

marcosmarxm added team/tse Technical Support Engineers community and removed community-legacy labels Jun 27, 2023

Merge branch 'master' into okta-group-members-stream

e6a7f0c

marcosmarxm and others added 2 commits July 7, 2023 11:38

Merge branch 'master' into okta-group-members-stream

d30b826

fix unit test and bump connector version

01d67f8

marcosmarxm approved these changes Jul 7, 2023

View reviewed changes

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Jul 7, 2023

fix formatting

580196b

marcosmarxm merged commit 89391de into airbytehq:master Jul 7, 2023
34 of 36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Source Okta: Fix Infinite Loop in GroupMembers Stream #20833

🐛 Source Okta: Fix Infinite Loop in GroupMembers Stream #20833

michaelcabiareyes commented Dec 22, 2022 •

edited

CLAassistant commented Dec 22, 2022 •

edited

koconder commented Dec 28, 2022 •

edited by github-actions bot

koconder commented Jan 1, 2023

michaelcabiareyes commented Jan 2, 2023

natalyjazzviolin commented Jan 18, 2023

michaelcabiareyes commented Jan 21, 2023

michaelcabiareyes commented Jan 22, 2023

koconder commented Jan 22, 2023

koconder commented Jan 22, 2023 •

edited by github-actions bot

michaelcabiareyes commented Jan 22, 2023

koconder commented Jan 22, 2023

michaelcabiareyes commented Jan 22, 2023

michaelcabiareyes left a comment

koconder commented Jan 23, 2023 •

edited by github-actions bot

michaelcabiareyes commented Jan 23, 2023 •

edited

koconder commented Jan 23, 2023

michaelcabiareyes commented Jan 29, 2023

koconder commented Jan 29, 2023 •

edited by github-actions bot

michaelcabiareyes commented Jan 30, 2023

Build Failed

michaelcabiareyes commented Jan 31, 2023

sh4sh commented Feb 9, 2023

michaelcabiareyes commented Feb 10, 2023

archangelic commented Jun 28, 2023

marcosmarxm left a comment

🐛 Source Okta: Fix Infinite Loop in GroupMembers Stream #20833

🐛 Source Okta: Fix Infinite Loop in GroupMembers Stream #20833

Conversation

michaelcabiareyes commented Dec 22, 2022 • edited

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Tests

CLAassistant commented Dec 22, 2022 • edited

koconder commented Dec 28, 2022 • edited by github-actions bot

Build Failed

koconder commented Jan 1, 2023

michaelcabiareyes commented Jan 2, 2023

natalyjazzviolin commented Jan 18, 2023

michaelcabiareyes commented Jan 21, 2023

michaelcabiareyes commented Jan 22, 2023

koconder commented Jan 22, 2023

koconder commented Jan 22, 2023 • edited by github-actions bot

Build Failed

michaelcabiareyes commented Jan 22, 2023

koconder commented Jan 22, 2023

michaelcabiareyes commented Jan 22, 2023

michaelcabiareyes left a comment

Choose a reason for hiding this comment

koconder commented Jan 23, 2023 • edited by github-actions bot

Build Failed

michaelcabiareyes commented Jan 23, 2023 • edited

koconder commented Jan 23, 2023

michaelcabiareyes commented Jan 29, 2023

koconder commented Jan 29, 2023 • edited by github-actions bot

Build Failed

michaelcabiareyes commented Jan 30, 2023

Build Failed

michaelcabiareyes commented Jan 31, 2023

sh4sh commented Feb 9, 2023

michaelcabiareyes commented Feb 10, 2023

archangelic commented Jun 28, 2023

marcosmarxm left a comment

Choose a reason for hiding this comment

michaelcabiareyes commented Dec 22, 2022 •

edited

CLAassistant commented Dec 22, 2022 •

edited

koconder commented Dec 28, 2022 •

edited by github-actions bot

koconder commented Jan 22, 2023 •

edited by github-actions bot

koconder commented Jan 23, 2023 •

edited by github-actions bot

michaelcabiareyes commented Jan 23, 2023 •

edited

koconder commented Jan 29, 2023 •

edited by github-actions bot