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

Airbyte-ci: Add path warning to check #33239

Merged
merged 6 commits into from
Dec 12, 2023
Merged

Airbyte-ci: Add path warning to check #33239

merged 6 commits into from
Dec 12, 2023

Conversation

bnchrch
Copy link
Contributor

@bnchrch bnchrch commented Dec 8, 2023

Problem

$HOME/.local/bin/airbyte-ci is not on the path by default

Solution

  1. During check ensure that it is
  2. If not output an error with instructions of how to resolve

@bnchrch bnchrch requested a review from a team December 8, 2023 01:12
Copy link

vercel bot commented Dec 8, 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 12, 2023 7:41pm

# If not print an error message and exit
if ! which airbyte-ci >/dev/null 2>&1; then
echo "airbyte-ci is not on the PATH"
echo "Check that \$HOME/.local/bin is part of the PATH"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I get what's exactly checking that \$HOME/.local/bin is part of the PATH
Could we have a prior step that just checks:

  • ~/.local/bin directory exists
  • That this path is part of the $PATH variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I had the absense of airbyte-ci as an indication of a improper path.

Update these to be separate checks

Copy link
Contributor

@erohmensing erohmensing left a comment

Choose a reason for hiding this comment

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

LGTM!

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Comment on lines +19 to +29
# Check that airbyte-ci is on the PATH
# If not print an error message and exit
if ! which airbyte-ci >/dev/null 2>&1; then
echo "airbyte-ci is not installed"
echo ""
echo "Please run 'make tools.airbyte-ci.install' to install airbyte-ci"
exit 1
fi

EXPECTED_PATH="$INSTALL_DIR/airbyte-ci"
AIRBYTE_CI_PATH=$(which airbyte-ci 2>/dev/null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nit: we could set this AIRBYTE_CI_PATH variable earlier, and check whether it is empty on L21 instead of running the command twice. E.g.

Suggested change
# Check that airbyte-ci is on the PATH
# If not print an error message and exit
if ! which airbyte-ci >/dev/null 2>&1; then
echo "airbyte-ci is not installed"
echo ""
echo "Please run 'make tools.airbyte-ci.install' to install airbyte-ci"
exit 1
fi
EXPECTED_PATH="$INSTALL_DIR/airbyte-ci"
AIRBYTE_CI_PATH=$(which airbyte-ci 2>/dev/null)
# Check that airbyte-ci is on the PATH
AIRBYTE_CI_PATH=$(which airbyte-ci 2>/dev/null)
# If not print an error message and exit
if [ -z "$AIRBYTE_CI_PATH" ]; then
echo "airbyte-ci is not installed"
echo ""
echo "Please run 'make tools.airbyte-ci.install' to install airbyte-ci"
exit 1
fi
EXPECTED_PATH="$INSTALL_DIR/airbyte-ci"

Comment on lines +22 to +24
echo "airbyte-ci is not installed"
echo ""
echo "Please run 'make tools.airbyte-ci.install' to install airbyte-ci"
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess my only question is "if I hit this right after (trying to) installing airbyte ci, then what?"

But i guess the answer is come talk to connector ops :D

Co-authored-by: Ella Rohm-Ensing <erohmensing@gmail.com>
@bnchrch bnchrch enabled auto-merge (squash) December 12, 2023 17:37
@bnchrch bnchrch merged commit b3a3fc0 into master Dec 12, 2023
19 checks passed
@bnchrch bnchrch deleted the bnchrch/ci/fix-clean branch December 12, 2023 19:46
rishabh-cldcvr pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Dec 14, 2023
Co-authored-by: Ella Rohm-Ensing <erohmensing@gmail.com>
tmathew0309 pushed a commit to tmathew0309/airbyte that referenced this pull request Jan 12, 2024
Co-authored-by: Ella Rohm-Ensing <erohmensing@gmail.com>
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.

None yet

3 participants