Skip to content

[dagster-airlift][components 2/n] scaffolder #29333

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

Open
wants to merge 1 commit into
base: airlift_instance_component
Choose a base branch
from

Conversation

dpeng817
Copy link
Contributor

@dpeng817 dpeng817 commented Apr 16, 2025

Summary & Motivation

Adds a components scaffolder for the airflow instance component.

In order to support multiple auth types from the same component, I'm taking auth_type as the top level parameter. However, this makes it difficult to scaffold out the actual underlying parameterization of the auth. Seems like something we might need additional tooling support for.

How I Tested These Changes

New components tests for python and yaml

Copy link
Contributor Author

dpeng817 commented Apr 16, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@dpeng817 dpeng817 requested review from OwenKephart and prha April 16, 2025 21:21
@dpeng817 dpeng817 marked this pull request as ready for review April 16, 2025 21:21
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from a091eef to bcdc03b Compare April 16, 2025 21:29
Comment on lines +48 to +56
if params.auth_type == "basic_auth":
full_params["auth"] = {
"type": "basic_auth",
"webserver_url": '{{ env("AIRFLOW_WEBSERVER_URL") }}',
"username": '{{ env("AIRFLOW_USERNAME") }}',
"password": '{{ env("AIRFLOW_PASSWORD") }}',
}
else:
raise ValueError(f"Unsupported auth type: {params.auth_type}")
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an inconsistency in the auth type handling. The AirflowInstanceScaffolderParams class accepts "mwaa" as a valid auth_type, but the scaffold method doesn't implement this case and will raise a generic error. Either implement the MWAA auth type scaffolding or update the error message to specifically indicate that MWAA scaffolding is not yet implemented.

Suggested change
if params.auth_type == "basic_auth":
full_params["auth"] = {
"type": "basic_auth",
"webserver_url": '{{ env("AIRFLOW_WEBSERVER_URL") }}',
"username": '{{ env("AIRFLOW_USERNAME") }}',
"password": '{{ env("AIRFLOW_PASSWORD") }}',
}
else:
raise ValueError(f"Unsupported auth type: {params.auth_type}")
if params.auth_type == "basic_auth":
full_params["auth"] = {
"type": "basic_auth",
"webserver_url": '{{ env("AIRFLOW_WEBSERVER_URL") }}',
"username": '{{ env("AIRFLOW_USERNAME") }}',
"password": '{{ env("AIRFLOW_PASSWORD") }}',
}
elif params.auth_type == "mwaa":
raise NotImplementedError(
"MWAA authentication type is recognized but scaffolding is not yet implemented"
)
else:
raise ValueError(f"Unsupported auth type: {params.auth_type}")

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

if params.auth_type == "basic_auth":
full_params["auth"] = {
"type": "basic_auth",
"webserver_url": '{{ env("AIRFLOW_WEBSERVER_URL") }}',
Copy link
Member

Choose a reason for hiding this comment

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

yea this is not ideal but seems reasonable enough for now

@dpeng817 dpeng817 force-pushed the airlift_instance_component branch from d8cf156 to c35ec7a Compare April 23, 2025 16:52
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from 6a55028 to e8909b2 Compare April 23, 2025 22:00
@dpeng817 dpeng817 force-pushed the airlift_instance_component branch from 445227c to 615618e Compare April 23, 2025 22:28
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from e8909b2 to e62152a Compare April 23, 2025 22:28
@dpeng817 dpeng817 force-pushed the airlift_instance_component branch from 615618e to a271a90 Compare April 23, 2025 23:09
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from e62152a to 195aefa Compare April 23, 2025 23:09
@dpeng817 dpeng817 force-pushed the airlift_instance_component branch from a271a90 to f932c8b Compare April 23, 2025 23:36
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from 195aefa to 47b9879 Compare April 23, 2025 23:36
@dpeng817 dpeng817 force-pushed the airlift_instance_component branch from f932c8b to 6b76aef Compare April 24, 2025 00:50
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from 47b9879 to 197b5f3 Compare April 24, 2025 00:51
@dpeng817 dpeng817 force-pushed the airlift_instance_component branch from 6b76aef to c4c4fd3 Compare April 24, 2025 00:59
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from 197b5f3 to 96d27bc Compare April 24, 2025 00:59
@dpeng817 dpeng817 force-pushed the airlift_instance_component branch from c4c4fd3 to ee04ae2 Compare April 24, 2025 04:19
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from 96d27bc to f94e74f Compare April 24, 2025 04:19
@dpeng817 dpeng817 force-pushed the airlift_instance_component branch from ee04ae2 to d650123 Compare April 24, 2025 04:28
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from f94e74f to 163b6f8 Compare April 24, 2025 04:29
@dpeng817 dpeng817 force-pushed the airlift_instance_component branch from d650123 to f046484 Compare April 24, 2025 05:06
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from 163b6f8 to fdea73e Compare April 24, 2025 05:06
@dpeng817 dpeng817 force-pushed the airlift_instance_component branch from f046484 to 9db0786 Compare April 24, 2025 23:47
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from fdea73e to 020ec70 Compare April 24, 2025 23:47
@dpeng817 dpeng817 force-pushed the airlift_instance_component branch from 9db0786 to 99dcb50 Compare April 25, 2025 00:15
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from 020ec70 to 056a789 Compare April 25, 2025 00:15
@dpeng817 dpeng817 force-pushed the airlift_instance_component branch from 99dcb50 to a3be2ff Compare April 25, 2025 00:29
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from 056a789 to 6c3a327 Compare April 25, 2025 00:29
@dpeng817 dpeng817 force-pushed the airlift_instance_component branch from a3be2ff to 190d814 Compare April 25, 2025 02:03
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from 6c3a327 to d4c19cc Compare April 25, 2025 02:06
This was referenced Apr 25, 2025
@dpeng817 dpeng817 force-pushed the airlift_instance_component branch from 190d814 to 0fb2a79 Compare April 29, 2025 00:46
@dpeng817 dpeng817 force-pushed the dpeng817/airlift_instance_scaffolder branch from d4c19cc to 902eb07 Compare April 29, 2025 00:46
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.

2 participants