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.

Sign up for GitHub

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

Compare beginning and end of whole PR when determining if dag-only #53

Merged
merged 4 commits into from
Feb 21, 2024
Merged

Compare beginning and end of whole PR when determining if dag-only #53

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6abd8c4
Compare beginning and end of whole PR
collinmcnulty Feb 13, 2024
b4b96b6
Removing code that isn't relevant to the general case
collinmcnulty Feb 13, 2024
b35fde8
Fetch commit from before the PR
collinmcnulty Feb 13, 2024
074f52e
Update action.yaml
sunkickr Feb 21, 2024
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
8 changes: 6 additions & 2 deletions action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,12 @@ runs:
with:
fetch-depth: 2
clean: false
- name: Fetch commit from before PR
run: |
get fetch origin ${{ github.event.before }}
sunkickr marked this conversation as resolved.
Show resolved Hide resolved
shell: bash
Copy link

Choose a reason for hiding this comment

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

This is probably not needed as you are not using any bash directives

# Determine if only DAGs have changes
- name: Install Astro
- name: Install Astro CLI
run: |
curl -sSL https://install.astronomer.io | sudo bash -s ${{ inputs.cli-version }}
shell: bash
Expand Down Expand Up @@ -264,7 +268,7 @@ runs:
- name: Get Deployment Type
run: |
cd ${{ inputs.root-folder }}
files=$(git diff --name-only HEAD^..HEAD)
files=$(git diff --name-only ${{ github.event.before }} ${{ github.event.after }})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if this will work with this change alone, because the clone on line 90 only fetches with a depth of two commits. Some options may be to fetch a reasonably large number of commits, say 50, which leaves a small risk of missing some in extreme cases, or to fetch all commits (I think the default if you don't specify fetch-depth, would need to be check that) which could be slow on very large repos, or to somehow use the push event payload to figure out the required fetch depth for that push (not sure if this is possible, but it would be the length of the commits list + 1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only a single commit is fetched by default, for the ref/SHA that triggered the workflow. Set fetch-depth: 0 to fetch all history for all branches and tags. Refer here to learn which commit $GITHUB_SHA points to for different events.

https://github.com/actions/checkout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could we just run git fetch origin ${{ github.event.before }} after checkout?

dags_only=1

for file in $files; do
Expand Down