dpguthrie / dbtc

39 stars 12 forks source link

Feat/abstract configs #38

Closed matt-winkler closed 1 year ago

codecov[bot] commented 2 years ago

Codecov Report

Base: 86.15% // Head: 84.61% // Decreases project coverage by -1.54% :warning:

Coverage data is based on head (db38153) compared to base (8b56460). Patch coverage: 71.00% of modified lines in pull request are covered.

:exclamation: Current head db38153 differs from pull request most recent head cfea827. Consider uploading reports for the commit cfea827 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #38 +/- ## ========================================== - Coverage 86.15% 84.61% -1.55% ========================================== Files 13 15 +2 Lines 1127 1183 +56 ========================================== + Hits 971 1001 +30 - Misses 156 182 +26 ``` | [Impacted Files](https://codecov.io/gh/dpguthrie/dbtc/pull/38?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Doug+Guthrie) | Coverage Δ | | |---|---|---| | [dbtc/client/cloud/configs/dbt\_cloud\_api.py](https://codecov.io/gh/dpguthrie/dbtc/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Doug+Guthrie#diff-ZGJ0Yy9jbGllbnQvY2xvdWQvY29uZmlncy9kYnRfY2xvdWRfYXBpLnB5) | `42.85% <42.85%> (ø)` | | | [dbtc/client/cloud/base.py](https://codecov.io/gh/dpguthrie/dbtc/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Doug+Guthrie#diff-ZGJ0Yy9jbGllbnQvY2xvdWQvYmFzZS5weQ==) | `77.77% <74.39%> (-2.82%)` | :arrow_down: | | [dbtc/cli.py](https://codecov.io/gh/dpguthrie/dbtc/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Doug+Guthrie#diff-ZGJ0Yy9jbGkucHk=) | `74.52% <100.00%> (+0.09%)` | :arrow_up: | | [dbtc/client/cloud/configs/dbt\_core\_cli.py](https://codecov.io/gh/dpguthrie/dbtc/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Doug+Guthrie#diff-ZGJ0Yy9jbGllbnQvY2xvdWQvY29uZmlncy9kYnRfY29yZV9jbGkucHk=) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Doug+Guthrie). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Doug+Guthrie)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

matt-winkler commented 2 years ago

@dpguthrie There's a lot in this PR, so to help organize thoughts I wanted to lay out a few areas where feedback would be valuable:

dpguthrie commented 2 years ago

The notion of templating our API requests in dbt_cloud_api

I came up with a different approach for validation using the pydantic library. This would both do validation of the payload that a user specifies as well as give it the appropriate defaults (id for instance).

https://github.com/dpguthrie/dbtc/pull/39

We can iterate on what's best, I just didn't want to overwrite all your work.

dpguthrie commented 2 years ago

As the number of use cases grows, the trigger_job method is going to have lots of possible parameters. Are we OK with that or would it be valuable to expose the concept of run modes in separate methods callable from the CLI, e.g. dbtc trigger-job-with-restart-from-failure, dbtc trigger-job-with-autoscale, etc.

Putting this here as well - my take is that we should be building out different methods that accomplish these different tasks. The trigger_job method is already probably a bit too big and doing too much. It should be the entry point to trigger a job from all those other methods, but each of those methods will have logic that doesn't make sense to wrap all together. Also, I'm guessing you're just gonna continue thinking of awesome use cases for this library, so we should plan for that.

matt-winkler commented 2 years ago

@dpguthrie 100 on breaking out separate methods that all get to the base trigger_job. I was mid-way on that before thinking to stop and ask you :))

matt-winkler commented 2 years ago

@dpguthrie Think I found a happier medium with the cloning idea. This updates _clone_resource so that it will return a payload that includes the object's required fields + and optional fields that are not None in the source data. Then we can further modify depending on the use case e.g. 1) in autoscaling we want to add an identifier to the job's name OR 2) if this was for migration, just push the same payload to another account / project etc.

matt-winkler commented 2 years ago

Also adding a comment that tests on autoscaling jobs are failing because we need to standardize on an account / jobs to work from.