-
Notifications
You must be signed in to change notification settings - Fork 97
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
refactor(airflow): Remove bootstrap_project
#599
Conversation
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
bootstrap_project
bootstrap_project
type=click.Path(writable=True, resolve_path=False, file_okay=False), | ||
default="airflow_dags/", |
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.
Why this change is needed?
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.
The target path shouldn't be resolved relative to the current directory at this point because it needs to be joined with the metadata.project_path
later on.
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.
looks good!
Signed-off-by: Ankita Katiyar <110245118+ankatiyar@users.noreply.github.com>
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
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.
LGTM, but left one question.
project_path = kedro_project.resolve() | ||
metadata = bootstrap_project(project_path) | ||
return metadata |
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.
Is this now necessary for the test setup because we don't call bootstrap_project
in the actual flow anymore?
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.
Yeah, since we don't do it in the plugin code, it's necessary for the unit tests to run.
* Remove bootstrap_project Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com> * Refactor Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com> * Fix tests Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com> * Print for debugging Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com> * add resolve Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com> * Update tests Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com> --------- Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com> Signed-off-by: Ankita Katiyar <110245118+ankatiyar@users.noreply.github.com> Signed-off-by: tgoelles <thomas.goelles@gmail.com>
Description
Following the discussion wrt kedro-org/kedro#3683
Development notes
bootstrap_project
and getproject_path
frommetadata
Checklist
RELEASE.md
file