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

Jump to bottom

airbyte-ci: Add --yes option #33206

Merged
merged 6 commits into from
Dec 7, 2023
Merged

airbyte-ci: Add --yes option #33206

merged 6 commits into from
Dec 7, 2023

Conversation

bnchrch
Copy link
Contributor

@bnchrch bnchrch commented Dec 6, 2023

Overview

Add a -y option to airbyte-ci to preagree to any prompts

@bnchrch bnchrch requested a review from a team December 6, 2023 23:16
@vercel Vercel
Copy link

vercel bot commented Dec 6, 2023 •
edited
Loading

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Dec 7, 2023 1:11am

evantahler
evantahler approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@evantahler evantahler left a comment

Choose a reason for hiding this comment

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

I think that it feels a little dangerous to link a local activity (upgrade the binary) with a remote activity that has downstream effects (publishing) with the same flag. e.g., I'll always want to run with -y locally to upgrade, but since the same flag means that I also might publish by accident... I guess I won't use it?

@bnchrch Graphite App
Copy link
Contributor Author

bnchrch commented Dec 7, 2023

@evantahler Fair call out.

Personally I wanted to keep a global -y flag as it mirrors what I expect after years of cli tools.

However I agree with your point of Local vs remote. Safe vs dangerous.

So I updated this to have a flag specifically for auto-update.

LMK if you thinks its an improvement

evantahler
evantahler approved these changes Dec 7, 2023
View reviewed changes
Copy link
Contributor

@evantahler evantahler left a comment

Choose a reason for hiding this comment

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

👍 works for me!

@bnchrch bnchrch merged commit 551f9ab into master Dec 7, 2023
16 checks passed
@bnchrch bnchrch deleted the bnchrch/ci/add-auto-update-flag branch December 7, 2023 17:19
@evantahler
Copy link
Contributor

evantahler commented Dec 7, 2023 •
edited
Loading

--yes should have been my approval message. I regret not doing that

rishabh-cldcvr pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Dec 14, 2023
@bnchrch @rishabh-cldcvr
airbyte-ci: Add --yes option (airbytehq#33206)
906f9c9 
Co-authored-by: bnchrch <bnchrch@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evantahler evantahler evantahler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bnchrch @evantahler