DataJunction / dj

A metrics platform.
http://datajunction.io
MIT License
35 stars 15 forks source link

Cleanup YAML project code. #1193

Closed agorajek closed 1 month ago

agorajek commented 1 month ago

Summary

Few things in this PR:

  1. Added update_if_exists flag to namespace and tag creation.
  2. Made the above flag default to False everywhere. It used to be True on the node, which I think is not great default. cc @anhqle
  3. Added system project namespace cleanup when run inside validation.

Test Plan

Unit tests and my local examples.

Deployment Plan

Asap.

netlify[bot] commented 1 month ago

Deploy Preview for thriving-cassata-78ae72 canceled.

Name Link
Latest commit 56d892746aebc1eb28e5ca61fd4ed394c37795aa
Latest deploy log https://app.netlify.com/sites/thriving-cassata-78ae72/deploys/67044a0ec8507f0008825977
agorajek commented 1 month ago

Changes largely LGTM!

Only one comment:

Made the above flag default to False everywhere. It used to be True on the node, which I think is not great default.

@agorajek From what I've seen, a lot of people seem to rely on this flag being true... of course we can announce that this is a breaking change, but I'm wondering if that's just the default behavior that people expect or prefer?

@shangyian I am worried that the current setup (created_node --> update_if_exists=True) will lead to silent overrides. I think I could do the following:

Thoughts?

agorajek commented 1 month ago

I'm good with changing the default of update_if_exists to False.

In that case, it'd be nice to tell user about the update_if_exists functionality when they run into node-already-exists-error.

Yes, we have that already, example:

datajunction.exceptions.DJClientException: Node `default.account_type_table` already exists. Use `update_if_exists=True` to update the node.