TDL-18878: Updated Endpoint for checklists stream #31
Conversation
…ards' of https://github.com/singer-io/tap-trello into TDL-16246-make-max-api-response-size-configurable-for-cards
hpatel41
changed the base branch from
master
to
TDL-16246-make-max-api-response-size-configurable-for-cards
tap_trello/streams.py
Outdated
| def __init__(self, client, config, state, catalog): | ||
| super().__init__(client, config, state, catalog) | ||
|
|
||
| # check is 'checklists' stream is selected |
There was a problem hiding this comment.
| # check is 'checklists' stream is selected | |
| # check if 'checklists' stream is selected |
tap_trello/streams.py
Outdated
|
|
||
| # check is 'checklists' stream is selected | ||
| self.is_checklists_selected = self.catalog.get_stream('checklists').is_selected() | ||
| # if it is selected, then create 'CheckLists' object |
There was a problem hiding this comment.
| # if it is selected, then create 'CheckLists' object | |
| # if checklists stream is selected, then create the 'CheckLists' object |
| } | ||
| } | ||
| }, | ||
| "email": { |
There was a problem hiding this comment.
Added email field for cards, as this stream was missing the field. As per this failure.
hpatel41
requested review from
KrisPersonal,
RushiT0122,
kspeer825,
nitingaikwad1 and
shantanu73
hpatel41
changed the base branch from
TDL-16246-make-max-api-response-size-configurable-for-cards
to
master
| elif stream == 'checklists': | ||
| # NOTE: We updated the endpoint for checklists stream | ||
| # as a result we will not replicate this 2 fields | ||
| # Please refere card: https://jira.talendforge.org/browse/TDL-18878 for more details | ||
| expected_fields.remove('limits') | ||
| expected_fields.remove('creationMethod') |
There was a problem hiding this comment.
If these fields are not going to ever be replicated, they should be removed from the schema. Another option would be to mark them as unsupported in metadata. I am not sure which solution makes the most sense here.
There was a problem hiding this comment.
@kspeer825 We have done changes for one customer as they are getting this error and it's got resolved with the alpha release.
That customer was fine if these two fields are not replicated with a newer approach so not sure if this change will be applied to all customers or not.
We can confirm it with Krishnan and also we will discuss like should we remove these fields from the schema or mark it as unsupported in metadata.
There was a problem hiding this comment.
Is there any update on this?
There was a problem hiding this comment.
No, @kspeer825 we will discuss with Krishnan in the next sync-up.
There was a problem hiding this comment.
@kspeer825 Krishnan suggested us to mark these fields as unsupported.
There was a problem hiding this comment.
In that case I think the discovery test will need to be updated to check that those fields are marked as unsupported in metadata.
There was a problem hiding this comment.
@kspeer825 The discovery test was not as per the standard, thus we updated the discovery test.
| # mark unsupported fields as 'unsupported' in the metadata | ||
| for field_name in stream.unsupported_fields: | ||
| metadata.write(mdata, ('properties', field_name), 'inclusion', 'unsupported') | ||
| metadata.write(mdata, ('properties', field_name), 'unsupported-description', \ |
There was a problem hiding this comment.
As per this code, we added description of unsupported fields.
| elif stream == 'checklists': | ||
| # NOTE: We updated the endpoint for checklists stream | ||
| # as a result we will not replicate this 2 fields | ||
| # Please refere card: https://jira.talendforge.org/browse/TDL-18878 for more details | ||
| expected_fields.remove('limits') | ||
| expected_fields.remove('creationMethod') |
There was a problem hiding this comment.
In that case I think the discovery test will need to be updated to check that those fields are marked as unsupported in metadata.
kspeer825
left a comment
kspeer825
left a comment
There was a problem hiding this comment.
Great work on the testing
Description of change
TDL-18878: Updated Endpoint for checklists stream
NOTE: We created this PR from
TDL-16246-make-max-api-response-size-configurable-for-cardsso all the changes are covered in this PR itself.Manual QA steps
Risks
Rollback steps