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

refactor: Rebuild from cookiecutter #104

Merged
merged 19 commits into from
Oct 28, 2024
Merged

refactor: Rebuild from cookiecutter #104

merged 19 commits into from
Oct 28, 2024

Conversation

pnadolny13
Copy link
Collaborator

@pnadolny13 pnadolny13 commented Oct 23, 2024

I hate to put a PR like this up but I had issues running the tap and making alterations to the current state was not working so I started fresh and moved one stream at a time as I tested that they worked. In the process I did the following:

  • moved to X-Restli-Protocol 2.0.0 by adding a way to include unencoded parameters. This was mentioned in the readme previously. Lots of url params and unencoded params needed to be updated after this to use new syntax.
  • removed all the required config IDs like campaign, owner, etc. because that limited the tap to a single subset of data. Instead we now use parent child relationships to iterate all available entities. In the future we may want to add some of those ids back in as optional filters.
  • there were lots of stream configurations that were not actually used and made the code confusing. For example there were replication keys and replication_method='incremental' which would not actually activate incremental syncs but the code reads as if it would. The replication keys werent being used anywhere in parameter filter that I could find.
  • Linkedin version bumped to 202404
  • Various bits of code cleanup like removing unused variables, replacing exception suppression, and making the code reuse the common code vs duplicating it all over.

TODOs or follow ups:

  • post processing of timestamps and other typed fields. I'm not sure how necessary this is.
  • AccountUsersStream seems to be broken right now. All others work
  • Clean up base streams
  • Find a better way to organize the analytics streams that need to be spread across multiple substreams then later merged to make the requests work.

Follow up issues:

@pnadolny13 pnadolny13 marked this pull request as ready for review October 25, 2024 21:22
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.

1 participant