search

AutoIDM / tap-clickup

tap-clickup , singer compliant tap for pulling clickup data
MIT License
12 stars 19 forks source link

Added a ref Resolver #105

Closed visch closed 2 years ago

visch commented 2 years ago

Closes #104

visch commented 2 years ago

Worried about potential json schema failures as we haven't been able to fully test this with Data in production setups (Tested with my test instance and one live instances data, but history shows that's not enough)

visch commented 2 years ago
saeedzareian commented 2 years ago

Hi, thank you for the great work and support here.

I am using list (folder-list) and space heavily and this fix will help us a lot. Also, we found another issue that I will share shortly with you after verifying my solution. Could you please move forward with this pull request earlier?

visch commented 2 years ago

Hi, thank you for the great work and support here.

I am using list (folder-list) and space heavily and this fix will help us a lot. Also, we found another issue that I will share shortly with you after verifying my solution. Could you please move forward with this pull request earlier?

Yeah I want to merge this, I just need to test that it works with target-postgres, and target-stitch. Issue is that I don't have functional testing setup to automate this for me (and I'd probably want to smoke test myself locally anyway)

Next steps are

  1. Verify data before and after this MR are the same in target-postgres (ie select from old_tap_clickup except select from new_tap_clickup and visaversa should return 0 records) , looks like CHECKSUM TABLE old_tap_clickup , new_tap_clickup ; would work too, if there are differences we should look at them and see if they make sense or not.
  2. Run tap-postgres target-stitch to be sure data gets through so I don't break anyone later
  3. Run tap-postgres target-jsonl to be sure schema checks pass

I won't have time to do this for a bit. I''ll try to, or if someone could do these tests for me or a portion of them it'd be mightly helpful!

visch commented 2 years ago

Hi, thank you for the great work and support here.

I am using list (folder-list) and space heavily and this fix will help us a lot. Also, we found another issue that I will share shortly with you after verifying my solution. Could you please move forward with this pull request earlier?

There's also some duplicate in the PR, I'd like to remove some of them and replace them with Refs, if someone could to that it would be very helpful as well (I hope it can work without changing the ref resolver we're using, if not we don't need to dive too hard we can just leave the duplication)

saeedzareian commented 2 years ago

I won't have time to do this for a bit. I''ll try to, or if someone could do these tests for me or a portion of them it'd be mightly helpful!

I will ask my friends if they have capacity to help, their sprint starts the next Monday (No tomorrow).

visch commented 2 years ago

Data looks good from a run on my end

  • [x] Check for any items in def's that don't' have a null option, add null as an option

After review there were only a handful of properties without nulls, this shouldn't' hold up the merge as this PR puts us in a better place.

visch commented 2 years ago

I won't have time to do this for a bit. I''ll try to, or if someone could do these tests for me or a portion of them it'd be mightly helpful!

I will ask my friends if they have capacity to help, their sprint starts the next Monday (No tomorrow).

We're all merged in, pull the latest from pypi and you should be good to go!