-
Notifications
You must be signed in to change notification settings - Fork 119
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
add secrets format option to dlt deploy #401
Conversation
✅ Deploy Preview for dlt-hub-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
9c45a5e
to
ae4e5f6
Compare
dlt/cli/_dlt.py
Outdated
|
||
# deploy airflow composer | ||
deploy_airlow_cmd = deploy_sub_parsers.add_parser(DeploymentMethods.airflow_composer.value, help="Deploys the pipeline to Airflow") | ||
deploy_airlow_cmd.add_argument("--secrets-format", default=SecretFormats.env, choices=[v.value for v in SecretFormats.__members__.values()], required=False, help="Format of the secrets") |
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.
you can iterate right over SecretFormats
[v.value for v in SecretFormats]
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.
cool thanks :)
dlt/cli/deploy_command.py
Outdated
self._echo_envs() | ||
elif self.secrets_format == SecretFormats.toml.value: | ||
# build toml | ||
fmt.echo(f"3. Add the following toml-string to the Airflow UI as the {SECRETS_TOML_KEY} variable.") |
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 "using Google Composer UI", we don't configure Airflow directly. but i'm not sure how exactly
@@ -42,14 +42,7 @@ def list_verified_sources_command_wrapper(repo_location: str, branch: str) -> in | |||
|
|||
|
|||
@utils.track_command("deploy", False, "deployment_method") | |||
def deploy_command_wrapper( |
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 the deployment_method
still passed? from my understanding this one and pipeline_script_name
must be always present so please declare them explicitly,
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.
also location and branch are always present
dlt/cli/_dlt.py
Outdated
@@ -285,7 +281,7 @@ def main() -> int: | |||
else: | |||
return init_command_wrapper(args.source, args.destination, args.generic, args.location, args.branch) | |||
elif args.command == "deploy": | |||
return deploy_command_wrapper(args.pipeline_script_path, args.deployment_method, args.schedule, args.run_on_push, args.run_manually, args.location, args.branch) | |||
return deploy_command_wrapper(**vars(args)) |
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 code is shorter but way harder to understand. IMO. some parameters are expected and should be explicitly passed
dlt/cli/deploy_command.py
Outdated
run_on_dispatch: bool, | ||
repo_location: str, | ||
branch: Optional[str] = None, | ||
def deploy_command(deployment_method: str, **kwargs: Any |
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.
same thing here
dlt/cli/deploy_command.py
Outdated
# build toml | ||
fmt.echo(f"3. Add the following toml-string to the Airflow UI as the {SECRETS_TOML_KEY} variable.") | ||
fmt.echo() | ||
toml_provider = MemoryTomlProvider("") |
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.
you can absolutely rename it!
1597716
to
113d70c
Compare
2646777
to
cc3155d
Compare
* add secrets format option to dlt deploy * fix linting * fix bug with missing args and separate github actions specific args out * pr fixes * fix test * improves deploy tests, fixes toml problem * fixes windows timer races due to low resolution: tx file tests * landing page for deploy a pipeline section --------- Co-authored-by: Marcin Rudolf <rudolfix@rudolfix.org>
Partly implements: #368
the deploy command for airflow-composer now includes a secrets-format option:
dlt deploy stripe_analytics_pipeline.py airflow-composer --secrets-format toml
which will result in a env output of the following if env or secrets are present: