Skip to content
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

Fix root folder reference #84

Merged
merged 19 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .github/workflows/tests.yaml
neel-astro marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,13 @@ jobs:
- name: Set CLI context
run: astro context switch ${{ steps.get-astro-env-info.outputs.astronomer_host }}

- name: Mock git commands for DAG Deploy
run: |

# set to no deploy git mock, to simulate no interested file change scenario
mv e2e-setup/mocks/no-deploy-git.sh /usr/local/bin/git
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixing a case that I found after merging my previous PR, as for the no-deploy case we want to simulate a change in a file outside the root folder. This is

chmod +x /usr/local/bin/git

- name: Get Deployment Info Before Test
id: get-deployment-before
if: ${{ matrix.deploy_type != 'dbt' }}
Expand Down
29 changes: 22 additions & 7 deletions action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ branding:
inputs:
root-folder:
required: false
default: ""
default: "./"
neel-astro marked this conversation as resolved.
Show resolved Hide resolved
description: "Path to the Astro project, or dbt project for dbt deploys."
parse:
required: false
Expand Down Expand Up @@ -308,8 +308,13 @@ runs:
echo "Files changed: $files"
DBT_DEPLOY=false

root_folder="${{ inputs.root-folder }}"
if [[ $root_folder == "./" ]]; then
root_folder=""
fi

for file in $files; do
if [[ $file =~ ^"${{ inputs.root-folder }}".* ]]; then
if [[ $file =~ ^"$root_folder".* ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need some kind of validation that the root folder does not start with a forward slash, which presumably would not match with any file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree on having the validation, also there are a couple of other places that could benefit from having upfront validation rather than having to fail later. But I would prefer to decouple that from this patch release so that we are focused on fixing bugs introduced in 0.7. (Will drop an issue so that we don't loose track of it)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue to track the change: #85

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we run astronomer/[email protected] prior to astro deployment variable update in CI. If there are no changes, this step fails. Will set the root-folder to see if that gets us moving forward. Ideally these changes could be versioned, if possible, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, since this is a fix, we are planning to release a v0.7.1 with these PR changes

echo $file is part of configured root folder, so would be triggering a dbt deploy
DBT_DEPLOY=true
fi
Expand Down Expand Up @@ -342,24 +347,34 @@ runs:
cd ${{ inputs.root-folder }}
branch=$(echo "${GITHUB_REF#refs/heads/}")
echo "Branch pushed to: $branch"
pwd
neel-astro marked this conversation as resolved.
Show resolved Hide resolved
git fetch origin $branch
files=$(git diff --name-only ${{ github.event.before }} ${{ github.event.after }})
echo "files changed: $files"
DAGS_ONLY_DEPLOY=false

DAGS_ONLY_DEPLOY=true
if [[ ${{ inputs.deploy-type }} == 'image-and-dags' ]]; then
DAGS_ONLY_DEPLOY=false
fi

SKIP_IMAGE_OR_DAGS_DEPLOY=true

root_folder="${{ inputs.root-folder }}"
if [[ $root_folder == "./" ]]; then
root_folder=""
fi

# This for loop checks for following cases:
# 1. If no file is part of the input root folder, then it skips deploy
# 2. If any file is not part of the dags folder, then it triggers a full image build
# 3. If all files are part of the dags folder and input root folder, then it triggers a DAG-only deploy
for file in $files; do
if [[ $file =~ ^"${{ inputs.root-folder }}".* ]]; then
if [[ $file =~ ^"$root_folder".* ]]; then
echo $file is part of the input root folder
SKIP_IMAGE_OR_DAGS_DEPLOY=false
if [[ $file == *"dags/"* && ${{ inputs.deploy-type }} != 'image-and-dags' ]]; then
echo $file is part of dags folder, triggering a DAG-only deploy
DAGS_ONLY_DEPLOY=true
if [[ $file != *"dags/"* ]]; then
echo $file is not part of dags folder
DAGS_ONLY_DEPLOY=false
neel-astro marked this conversation as resolved.
Show resolved Hide resolved
break
fi
fi
Expand Down
13 changes: 13 additions & 0 deletions e2e-setup/mocks/no-deploy-git.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#!/bin/bash

# hack to mock git commands as part of action.yaml so that we could simulate no deploy scenario without making any additional commits

# Check if the script was invoked with "git diff"
if [[ "$1" == "diff" ]]; then
echo "README.md"
elif [[ "$1" == "fetch" ]]; then
echo "Handling git fetch, doing nothing"
else
echo "Error: git mock script isn't configured to handle $1" >&2
exit 1
fi