Skip to content

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

@dbshah1212 dbshah1212 requested a review from cosimon September 6, 2022 13:49
"""
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.

@dbshah1212 dbshah1212 requested a review from cosimon September 7, 2022 11:48
@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.

7 participants