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

Features/dlt deploy airflow #356

Merged
merged 40 commits into from
May 29, 2023
Merged

Features/dlt deploy airflow #356

merged 40 commits into from
May 29, 2023

Conversation

AstrakhantsevaAA
Copy link
Contributor

No description provided.

@netlify
Copy link

netlify bot commented May 24, 2023

Deploy Preview for dlt-hub-docs canceled.

Name Link
🔨 Latest commit 0b3f4c5
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/6474b59c8203570008b5e7b3

Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

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

looks good! most of my comments are to write proper object oriented code (because you already defined class for the deployment and general direction is good)

dlt/cli/_dlt.py Show resolved Hide resolved
dlt/cli/deploy_command.py Show resolved Hide resolved
dlt/cli/deploy_command.py Outdated Show resolved Hide resolved
dlt/cli/deploy_command.py Outdated Show resolved Hide resolved
dlt/cli/deploy_command.py Outdated Show resolved Hide resolved
dlt/cli/deploy_command.py Outdated Show resolved Hide resolved
dlt/cli/deploy_command.py Outdated Show resolved Hide resolved
@AstrakhantsevaAA AstrakhantsevaAA marked this pull request as ready for review May 26, 2023 16:20
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

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

looks good and ready to merge. parametrize the existing test was cool. I did following changes:

  1. renamed to airflow-composer: we require github repo and distribute cloudbuild. if we need vanilla airflow we can add that
  2. any modifications should be made only after instructions are displayed: so there's another method in the base class (_make_modifications) being called at the end
  3. you removed the code that displays actual env variables, I added those back...

@rudolfix rudolfix merged commit f22963c into devel May 29, 2023
@rudolfix rudolfix deleted the features/dlt_deploy_airflow branch May 29, 2023 15:55
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

4 participants