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.

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

TDL-20481 Update pks in schema of users stream #95

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

prijendev
Copy link
Contributor

@prijendev prijendev commented Sep 6, 2022

Description of change

  • Updated primary keys value before writing schema for users stream in sync mode.

Manual QA steps

  • Run discover mode in the tap and verify that key-properties is key of users stream for on-prem Jira instance in the catalog.
  • Run sync mode in the tap and verify that key-properties is updated to key in schema message of users stream for on-prem Jira instance.

Risks

Rollback steps

  • revert this branch

Comment on lines 96 to 98
# Update pk for users stream to key for on prem jira instance
if stream.tap_stream_id == "users" and Context.client.is_on_prem_instance:
stream.pk_fields = ["key"]
Copy link
Contributor

Choose a reason for hiding this comment

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

We already do this in generate_metadata(), can we refactor so we only have 1 place in code in which we generate our schema?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

"""
if Context.client.is_on_prem_instance:
# ALL_STREAMS contains `Users` object at index 7
streams_.ALL_STREAMS[7].pk_fields = ["key"]
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not be making the ordering of the streams in ALL_STREAMS matter. Just call discover() in the sync function to get the catalog instead of loading the schema in 2 different places in code.

Copy link
Contributor

Choose a reason for hiding this comment

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

We were already calling discovery but not using generated catalog which we updated.

@shantanu73 shantanu73 self-requested a review November 25, 2022 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants