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

Handle case when triggered event is a manual workflow_dispatch #97

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sakethsomaraju
Copy link

Fix #94 and #91

Added support to perform "Image Deploy" incase of manual_dispatch github event.

@sakethsomaraju sakethsomaraju requested a review from a team as a code owner November 29, 2024 11:01
Copy link
Collaborator

@neel-astro neel-astro left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking this up, highly appreciate it 👏
Have we tried out new changes manually? If not, could we do that, and add the result to the PR description?
You could point to this deploy-action branch in the GitHub workflow by specifying the deploy action reference, ex: uses: astronomer/deploy-action@fix/94

Apart from that, I left some minor comments to keep things clean.

Comment on lines +380 to +381
GITHUB_EVENT_BEFORE=${{ github.event.before }}
GITHUB_EVENT_AFTER=${{ 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.

Can we reuse these variables for the rest of the logic in this step?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for taking time to review. :-) Addressed.

Comment on lines +319 to +320
GITHUB_EVENT_BEFORE=${{ github.event.before }}
GITHUB_EVENT_AFTER=${{ 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.

Can we reuse these variables for the rest of the logic in this step?

Copy link
Author

Choose a reason for hiding this comment

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

Addressed

action.yaml Outdated
# case when the triggered event is a manual workflow dispatch, we would need to deploy the dbt project because we cannot determine that it does not need to be deployed
GITHUB_EVENT_BEFORE=${{ github.event.before }}
GITHUB_EVENT_AFTER=${{ github.event.after }}
if [[ -z $GITHUB_EVENT_BEFORE && -z $GITHUB_EVENT_AFTER ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: could we club both the if clause (this and the one below) into a single clause to avoid code repetition :)

Copy link
Author

Choose a reason for hiding this comment

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

Addressed

action.yaml Outdated
# case when the triggered event is a manual workflow dispatch, we would need to deploy the image because we cannot determine that it does not need to be deployed
GITHUB_EVENT_BEFORE=${{ github.event.before }}
GITHUB_EVENT_AFTER=${{ github.event.after }}
if [[ -z $GITHUB_EVENT_BEFORE && -z $GITHUB_EVENT_AFTER ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: could we club both the if clause (this and the one below) into a single clause to avoid code repetition :)

Copy link
Author

Choose a reason for hiding this comment

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

Addressed

@sakethsomaraju
Copy link
Author

sakethsomaraju commented Dec 5, 2024

TESTING

Tested with following workflow

name: Personal Astronomer CI - Deploy
on:
  push:
    branches:
      - main
  workflow_dispatch:

env:
  ## Sets Deployment API credentials as environment variables
  ASTRO_API_TOKEN: ${{ secrets.ASTRO_API_TOKEN }}
  WORKSPACE: "clilt7vco00a601kk5fz65ob1"  
  DEPLOYMENT_ID: "cm3spbuo00nt301ni2wggd49b" 

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
    - name: Deploy to Astro DAGS
      # if: ${{ github.event.inputs.deploy_type == 'DAGS' }}
      uses: astronomer/deploy-action@address-review-comments
      with:
        deployment-id: ${{ env.DEPLOYMENT_ID }}
        workspace: ${{ env.WORKSPACE }}
        force: true
        checkout: true

Both Manual trigger and CI trigger were success.

Manual trigger reference:

image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not able to deploy with v0.8.0 using either workflow dispatch nor a merged PR trigger
2 participants