feat: Implement parent-child sync for all accessible customers #45
Conversation
|
@DanielPDWalker all done! |
|
@DanielPDWalker also need to make sure to update the config in the meltano hub to make sure the documentation is correct. |
tobiascadee
force-pushed
the
tobias/fix-parent-child-setup
branch
from
d607f2b to
b06eb21
Compare
|
@tobiascadee Your force-push has blown away my commits. Am I OK to add them back? |
|
@ReubenFrankel ah sorry I thought I would just clean up and integrate your commits into one. Of course you can. |
Ah, I see. Sorry, I didn't realise that's what you had done. 😅 Don't worry about it then. |
tobiascadee
force-pushed
the
tobias/fix-parent-child-setup
branch
from
b06eb21 to
39cb5a8
Compare
|
Pretty much there I think - can you update the README with the config changes? |
tobiascadee
force-pushed
the
tobias/fix-parent-child-setup
branch
2 times, most recently
from
a94595f to
35c3f90
Compare
|
@ReubenFrankel done |
There was a problem hiding this comment.
@tobiascadee I spoke with @DanielPDWalker some more this afternoon about the change here to make login_customer_id config required, and we don't think it needs to be. What was your motivation for making it required initially? I see that in the PR description you wrote
login_customer_idis now required as it is required in the header
but the API docs suggest that it is only required when authenticated as a manager account (from OAuth credentials) and accessing a separate customer account:
If accessing an operating customer account directly, the
login-customer-idheader is not required.
If the authenticated user has access to both the manager and operating customer accounts, then
login-customer-idshould be specified to avoid any ambiguity. Though not specifyinglogin-customer-idwould still work in this case, the context is set to the operating customer account only (which is probably not what is intended).
I don't think there's a great way for the tap to know whether or not you are authenticated in such a way to not set the header at all, so the previous implementation would always set it to either the optional login_customer_id config or the subject customer_id otherwise (see #32). Since this is a tried and tested approach, I think we should re-implement it here to use either login_customer_id or the subject customer_id from context (rather than config).
If we do go ahead with this, you can undo your README changes as well (sorry).
FYI, I am away today so will probably not get back to this until Thursday.
At the moment `AccessibleCustomers` is not being used as a parent stream to `CustomerHierarchyStream`. This update fixes that. By updating to the latest singer-sdk version we can use `generate_child_contexts` to generate multiple child streams per parent stream to sync all accessible customers. I also added a check to make sure a customer is enabled to avoid 403 errors. Also changed some of the tap config because in my opinion it was a bit unclear. `login_customer_id` is now required as it is required in the header. You can also specify `customer_ids` to filter on customers you want to sync. But only if you have access to them and they are enabled. I understand that some people need to update their config but in the end I think this is more clear. Moved pagination to the new singer-sdk pagination setup: https://sdk.meltano.com/en/v0.40.0/guides/pagination-classes.html Other updates are linting updates.
tobiascadee
force-pushed
the
tobias/fix-parent-child-setup
branch
from
e4e5696 to
9840e5c
Compare
|
I also moved the path and gaql methods to the base class because they are also used in the |
ReubenFrankel
changed the title
Co-authored-by: ReubenFrankel <[email protected]>
Co-authored-by: ReubenFrankel <[email protected]>
|
@ReubenFrankel your suggestion works on my end. annoying that they implemented it in this way but I guess its the one of the cleanest solutions. any other remarks? |
|
@ReubenFrankel can we merge? |
|
Right, let's get this in then. Thanks for being patient @tobiascadee |
At the moment
AccessibleCustomersis not being used as a parent stream toCustomerHierarchyStream. This update fixes that. By updating to the latest singer-sdk version we can usegenerate_child_contextsto generate multiple child streams per parent stream to sync all accessible customers.I also added a check to make sure a customer is enabled to avoid 403 errors.
Also changed some of the tap config because in my opinion it was a bit unclear.
login_customer_idis now required as it is required in the header. You can also specifycustomer_idsto filter on customers you want to sync. But only if you have access to them and they are enabled. I understand that some people need to update their config but in the end I think this is more clear.Moved pagination to the new singer-sdk pagination setup: https://sdk.meltano.com/en/v0.40.0/guides/pagination-classes.html
Other updates are linting updates.