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

Update airbyte-ci action to install dev if pipeline has been edited #33519

Merged
merged 25 commits into from
Dec 20, 2023

Conversation

bnchrch
Copy link
Contributor

@bnchrch bnchrch commented Dec 15, 2023

Overview

  1. Updates our airbyte-ci github action to download the dev version if piplines has been modified
  2. Updates our airbyte-ci release action to always run on PRs that modify pipelines

closes #33418

Pain points

  1. Github doesn't consitently give you the sha that triggered the event: 1, 2
  2. The shorten sha action didnt account for this or let us override the sha it was shortening (so I removed it)
  3. Waiting on a github workflow to complete depended on waiting on its check name (not filename) which felt brittle so I opted for polling instead.
  4. These are github actions, the change and verify cycle is LONG, so lets focus on done not perfect with this one.

Copy link

vercel bot commented Dec 15, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 20, 2023 4:57pm

@bnchrch bnchrch requested a review from a team December 15, 2023 01:22
@bnchrch bnchrch force-pushed the bnchrch/ci/airbyte-ci-dev-pipeline branch 6 times, most recently from fdeaa9b to 1ba7845 Compare December 16, 2023 00:08
@bnchrch bnchrch changed the title Draft: Update airbyte-ci action to install dev if pipeline has been edited Update airbyte-ci action to install dev if pipeline has been edited Dec 16, 2023
Comment on lines 9 to 10
branches:
- master
paths:
- "airbyte-ci/connectors/pipelines/**"
pull_request:
types:
- opened
- reopened
- synchronize
paths:
- "airbyte-ci/connectors/pipelines/**"
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not mistaken the release will be triggered on airbyte-ci file changes only.
So if you make a change to a non airbyte-ci related file on a later commit the run-dagger-pipeline/action.yml will never resolve a binary for this commit.

I think that a dev install with pipx when the PR (not the commit) has airbyte-ci modified files is easier to implement.

Another option would be to always trigger this workflow (no paths filter) and use tj-actions/changed-files to only trigger a release if the branch has airbyte-ci modified files.

I'm not sure if tj-actions/changed-files returns the changed-files compared to the previous commit or compared to the target branch

Copy link
Contributor Author

@bnchrch bnchrch Dec 19, 2023

Choose a reason for hiding this comment

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

If I'm not mistaken the release will be triggered on airbyte-ci file changes only.

So interestingly enough this was the case previously when using push.paths. However its not the case for pull_request.paths + pull_request.types=synchronize, which will fire everytime the branch is update and there is a change matching the path in anyone of the commits that make up the branch

I think that a dev install with pipx when the PR (not the commit) has airbyte-ci modified files is easier to implement.

I think your right. I went down this path as it would be useful to test pipeline changes using the final pyinstaller executable. As this is what the end users actually run.

But looking at this now, the steps we take to poll for new dev binaries that match your commit is brittle and more complex than we should tolerate.

TLDR: Ill update to use the pipx dev install instead

@bnchrch bnchrch force-pushed the bnchrch/ci/airbyte-ci-dev-pipeline branch from 07f2988 to 7f98892 Compare December 19, 2023 21:48
@bnchrch bnchrch requested a review from alafanechere December 19, 2023 21:55
@bnchrch
Copy link
Contributor Author

bnchrch commented Dec 19, 2023

@alafanechere back up and ready for review. We no longer are using the dev executables but instead the pipx install. Much simpler.

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

Thanks! I confirm it works: I opened a PR based on this one on which I've modified pipeline code -> the formatting workflow installs airbyte-ci with pipx

@bnchrch bnchrch enabled auto-merge (squash) December 20, 2023 16:55
@bnchrch bnchrch merged commit 84a8b26 into master Dec 20, 2023
17 checks passed
@bnchrch bnchrch deleted the bnchrch/ci/airbyte-ci-dev-pipeline branch December 20, 2023 17:03
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
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.

Update ci to use dev versoin of airbyte-ci when pipeline is edited
2 participants