Decouple hook executor from the operator, add Cloud Build execution and adjust testing #48
Conversation
…nd adjust testing
The Airflow bash executor will run the command isolated in the worker. To be able to locate the profiles and code we have to tell dbt with the new implemented flag `dbt run --project-dir /path/to/my/project_dir --profiles-dir /path/to/profile.yaml` `
That comes useful when running the command inside cloud run, since the entrypoint for the command is already `dbt`, and this `dbt` is already in the PATH of the image
Local execution might not take place in the same folder. It depends on the underlying hook implementation. This way we ensure we're pointing to the folder with an absolute path even if run outside the project folder
Also make the command build function more idiomatic by using `append` to append single commands (flags) and `extend` when we want to append several.
Also fix imports and fix a dbt cli test
|
Hey @dinigo, Thanks for the contribution! This looks really good and it's great you've been able to make these changes without changing the API. I've had a quick look through the code and it looks good. Given there are quite a few changes I want to have another look tomorrow and test it a bit more, but not expecting any problems. I'll get back to you by the end of day tomorrow. Thanks again! |
* dbt_bin is not needed other than to build the command, it's not necessary for the hook to have it in the constructor. * Operator templated fields are interpolated after the constructor runs. We're instantiating the hooks when the "executor" is called. Otherwise the hook has to be provided already instantiated.
dinigo
changed the title
|
There's still some work going on as you can see |
|
@dinigo is this ready for final review? |
|
Bump! Would there be interest in pulling out the environment variable functionality into a new PR so we can close #36? |
Hi @RowdyHowell, I was able to create a PR #60 with the env functionality, please take a look and feel free to leave a 👍 |
|
It's been some time since I put some time into this issue. I think the PR is too big, I'm glad some other people took the time to implement the issues separated, like the env variables. This PR addresses quite too many things in one go:
Each of those deserves (in my opinion) to be implemented, however they might require to be split into smaller PRs for us to review easily. This has been stalled for some time now and I think it is due to the complexity in the review process. Difficult to approve so many changes at once. What do you (core devs, people from GoCardless and from the community) think? Could you give a hand in the thinking process? I've been using these features and they work fine, however that's not enough. They also have to look good. Tell me what you think and If I could source some of your time |
|
Hey @dinigo, Sorry also from outside for not taking a look at these contributions sooner, which is partly why they have grown so big. I agree this is too hard to review now, whereas smaller PRs like #60 are much easier. I think all the changes you are proposing are the right ones to do (from your bullet points above), so if we had PRs for each we should be able to review and merge them. Thanks! |
|
@andrewrjones , I have split some of the functionality in three other PRs, namely #67 , #68 and #69. Prior to do anything else I need some assessment on them |
Follow #45 which was closed and locked when I accidentally renamed the branch it was pointing to
Closes #46, #44, #43, #36, #30