-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add DBT Deploy support #74
Conversation
1b28ccb
to
649765e
Compare
6c52997
to
c7900c8
Compare
@@ -96,7 +97,7 @@ jobs: | |||
runs-on: ubuntu-latest | |||
steps: | |||
- name: Deploy to Astro | |||
uses: astronomer/deploy-action@v0.4 | |||
uses: astronomer/deploy-action@v0.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update these to 0.7 once this PR is merged and we cut a new release
shell: bash | ||
id: deployment-preview | ||
- name: Determine if DAG Deploy is enabled | ||
run: | | ||
|
||
echo ::group::Determine if DAG Deploy is enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the group
& endgroup
allows GHA to group these logs
skip_deploy=1 | ||
|
||
# This for loop checks for following cases: | ||
# 1. If no file is part of the input root folder, then it skips deploy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to accommodate for the case when dbt project and astro project are part of the same repo, we would need to ensure that we only trigger image deploy when any file is updated inside the astro project's root folder, this is slightly different from existing behaviour where if any file outside the dags
folder is updated we trigger an image deploy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes sense! since these deploy-action releases are versioned, I'm comfortable with that and I wouldn't expect any breaking changes from it... in fact this might be better for users
action.yaml
Outdated
exit 1 | ||
fi | ||
|
||
# validate that mount path is provided |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want the mount path to be required here? Can we instead rely on the default mount path of the dbt Deploy (i.e. the directory name)?
Also noticed the mount-path input above not required (
Line 85 in c7900c8
mount-path: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @jeremybeard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mount path is not required by the feature and ideally we should be encouraging users not to set it because the CLI already provides a sensible default.
README.md
Outdated
@@ -48,11 +48,11 @@ The following table lists the configuration options for the Deploy to Astro acti | |||
|
|||
| Name | Default | Description | | |||
| ---|---|--- | | |||
| `action` | `deploy` | Specify what action you would like to take. Use this option to create or delete deployment previews. Specify either `create-deployment-preview`, `delete-deployment-preview` or `deploy-deployment-preview`. Don't sepcify anything if you are deploying to a regular deployment. | | |||
| `action` | `deploy` | Specify what action you would like to take. Use this option to create or delete deployment previews. Specify either `create-deployment-preview`, `delete-deployment-preview`, `deploy-deployment-preview` or `dbt-deploy`. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dbt-deploy-deployment-preview
is also being added right? Instead of adding two new actions would it make sense to just add an option dbt-deploy
that can be true/false?
README.md
Outdated
with: | ||
deployment-id: <deployment id> | ||
root-folder: dbt/ | ||
mount-path: /dbt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between mount-path and root-folder? would they always be the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are both features of dbt deploys.
mount-path -> the directory where the dbt code is actually mounted in your container
root-folder -> the directory where the dbt code is retrieved from
@neel-astro just dropping by to say kudos on a very well scoped PR with high quality description and leaving the codebase a little cleaner than you found it without distracting from the PR. I learned more about this feature by lurking here :) |
65e9967
to
203064b
Compare
405cfbc
to
495df2a
Compare
4ecbd17
to
a7ad4dd
Compare
f902443
to
7f3b65d
Compare
3fb9bd2
to
ff570e4
Compare
b7d8981
to
954b90f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @neel-astro, this is much simpler with the deploy type restructure. I left some comments, a few around all the output variables that I hope we can simplify even further.
dbt-deploy-test: | ||
name: DBT Deploy Test | ||
runs-on: ubuntu-latest | ||
needs: [custom-docker-image-test, create-test-deployments] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test depend on custom-docker-image-test
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does not depend on the custom-docker-image-test
, but because these tests are running against a single deployment, to avoid any flakiness in test results, we are running a single test at a time, and hence we are required to use the needs
clause to create the dependencies to avoid concurrent test scenario runs
action.yaml
Outdated
if [[ "${{ inputs.deploy-type }}" == "dbt" && $CLI_VERSION != "" ]]; then | ||
REQUIRED_VERSION="1.28.1" | ||
CURRENT_VERSION=$(astro version | awk '{print $4}') | ||
if [[ $CURRENT_VERSION =~ ([0-9]+)\.([0-9]+)\.([0-9]+) ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks robust. Another option might be to run npm install -g semver
and use that but you've already done it this way so we don't have to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good callout 👍 , I moved it to semver which reduces the logic
action.yaml
Outdated
@@ -257,62 +313,139 @@ runs: | |||
echo "IMAGE_DEPLOY_ONLY=false" >> $GITHUB_OUTPUT | |||
|
|||
# set action | |||
ACTION=deploy | |||
ACTION=${{ inputs.action }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this variable needed anymore? The only place I can see it used is at the end of the step to check the user provided a correct input, but we should have that at the start of the workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dropped the ACTION
variable, instead moved the corresponding validation check to the top of the function
action.yaml
Outdated
@@ -257,62 +313,139 @@ runs: | |||
echo "IMAGE_DEPLOY_ONLY=false" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also still need this variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And SKIP_DEPLOY
? That's only true if ${{ inputs.action }} == delete-deployment-preview
but we could just look at that?
action.yaml
Outdated
dags_only=0 | ||
fi | ||
|
||
if [[ ${{ steps.deployment-preview.outputs.SKIP_DEPLOY }} == true ]]; then | ||
# skip all deploy steps | ||
dags_only=2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pre-existing use of "2" for a boolean variable is... not ideal. I know this is creeping the scope but can we change these to use "true"/"false" and if we need to express other things like skipping the deploy then can we just output a second variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense, I had an itch as well, so replaced the numbering with boolean for both dags_only and dbt_only
shell: bash | ||
id: deployment-type | ||
# If only DAGs changed and dag deploys is enabled, do a DAG-only deploy | ||
- name: setup deploy options | ||
- name: Setup image or dags deploy options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this step need to be separate to the one before it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could merge them, but both are decently sized bash scripts, so we should avoid merging them unless we see a value addition by doing so otherwise it would be another very long script to manage. This function is well-scoped, and the other one isn't, and I agree with that.
dad9bf6
to
cdb51e1
Compare
9f73bcf
to
959cdf3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again, this is excellent. I've left one nitpick but approving ahead of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work
@neel-astro Do you know when you would be publishing a release with this feature? I was looking forward to using this. |
@neelavarrao trying to get it out in the next couple of days, thanks for the interest 🙂 |
Changes:
deploy-type
input to help distinguish between different forms of deploys, i.e. dags-only, dags-and-image, dbt and infer (to avoid introducing breaking change).dev-ex
team as the project code ownersMinor Fixes:
Closes: astronomer/astro#23047
Testing:
Updated E2E tests to include sample DBT deploys
GHA Template I have been using for the tests: