-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Allow airbyte-ci to run from anywhere in project #31412
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Hm, I'm afraid that being booted out of your working directory might not really solve the problem here - it still slows down the interaction cycle pretty significantly. That's an assumption, but something that might be worth asking our most receptive connector devs about. If we could get around the setting it teice, is there a way we could change directory back to where we were at the end? Maybe some sort of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent DX improvement. My comments are all nits.
def get_airbyte_repo() -> git.Repo: | ||
"""Get the airbyte repo.""" | ||
return git.Repo(search_parent_directories=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we throw if we aren't in a git repo at all?
That's done by the caller below 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we validate that the git repo we are in is, in fact, airbytehq/airbyte
?
@@ -180,6 +203,7 @@ def airbyte_ci( | |||
airbyte_ci.add_command(connectors) | |||
airbyte_ci.add_command(metadata) | |||
airbyte_ci.add_command(test) | |||
set_working_directory_to_root() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably fine, but do any child-commands or forks need to be in a certain directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. Currently every airbyte-ci
command is imported here and every airbyte-ci logic assumes airbyte/
as the working directory.
|
||
def set_working_directory_to_root() -> None: | ||
"""Set the working directory to the root of the airbyte repo.""" | ||
os.chdir(get_airbyte_repo_path_with_fallback()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if we are changing the CWD, we should probably log something, like "Using /path/to/root as working directory"
@erohmensing I think theres a small misunderstanding about what Essentially each process has its own "working directory" meaning that the python program we run, wont change the directory that the terminal is targeting. Its both why we dont have to reset the working directory back to the orginal and why I have to call
|
8f28f41
to
7dc9d32
Compare
@bnchrch thanks for the explanation, that makes sense! I'll admit I was only going off of the PR description, I hadnt checked it out and tried it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
||
If this command is run from outside the airbyte repo, it will not work properly. | ||
|
||
Please run this command your local airbyte project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run this command your local airbyte project. | |
Please run this command in your local airbyte project. |
|
||
logging.warning(warning_message) | ||
|
||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do? Looks like the return value is not used
Problem
You must run airbyte-ci from root
closes #31250
Solution
Just change the working directory when you run airbyte-ci to be root
Note for reviewer
Why changing the working directory instead of refactoring for absolute paths
Using absolute paths seemed more prone to error and a larger refactor
Example here: https://github.com/airbytehq/airbyte/pull/31409/files
Why do you set the working directory in two places
Because
dagger run
requires it set before invokedand
airbyte-ci-internal
when run on its own also needs it set