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

[dagster-airlift] AirflowInstance #23343

Merged
merged 1 commit into from
Aug 4, 2024
Merged

Conversation

dpeng817
Copy link
Contributor

@dpeng817 dpeng817 commented Jul 31, 2024

Add AirflowInstance API, which vends out access to the airflow rest API.

This paves the way for having multiple auth backends supporting airlift utilities. Also just happens to feel nicer api-wise.

@dpeng817 dpeng817 changed the title [dagster-airlift] airflow instance API [dagster-airlift] AirflowInstance Jul 31, 2024
@dpeng817 dpeng817 marked this pull request as ready for review July 31, 2024 05:45
@dpeng817 dpeng817 requested a review from schrockn July 31, 2024 05:47
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

Makes sense. This will be a nice seem for testability as we can make a fake for it.

return session


class AirflowInstance(NamedTuple):
Copy link
Member

Choose a reason for hiding this comment

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

seems a little goofy to have this be a NamedTuple but nbd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what's goofy about it being a namedtuple? I guess just vs a regular class you mean?

Honestly it's just learned behavior from the codebase at this point

Copy link
Member

Choose a reason for hiding this comment

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

Generally NamedTuple are "dumb" value objects that represent a value/record at a point in time. This is a more complex object that will likely accumulate state, require caching, that sort of thing.

Base automatically changed from dpeng817/fix_materialization_reporting_order to master August 3, 2024 23:52
@dpeng817 dpeng817 force-pushed the dpeng817/factor_out_instance branch from fa5a0dd to 264168d Compare August 3, 2024 23:59
@dpeng817 dpeng817 merged commit 4a49d75 into master Aug 4, 2024
1 check passed
@dpeng817 dpeng817 deleted the dpeng817/factor_out_instance branch August 4, 2024 00:13
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