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-dbt][DA] Add get_automation_condition method to DagsterDbtTranslator #23389

Conversation

OwenKephart
Copy link
Contributor

Summary & Motivation

Adds a get_automation_condition method to DagsterDbtTranslator, and updates some docs references.

How I Tested These Changes

@graphite-app graphite-app bot added the area: docs Related to documentation in general label Aug 2, 2024
@graphite-app graphite-app bot requested a review from erinkcochran87 August 2, 2024 21:03
Copy link
Contributor Author

OwenKephart commented Aug 2, 2024

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

Join @OwenKephart and the rest of your teammates on Graphite Graphite

@OwenKephart OwenKephart requested review from rexledesma and sryza August 2, 2024 21:04
@OwenKephart OwenKephart force-pushed the 08-02-_dagster-dbt_DA_Add_get_automation_condition_method_to_DagsterDbtTranslator branch from 3044511 to 5047565 Compare August 2, 2024 21:36
Copy link
Contributor

@rexledesma rexledesma left a comment

Choose a reason for hiding this comment

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

Can we remove get_auto_materialize_policy and associated tests as well? We're removing those docs anyways.

Copy link
Contributor

@rexledesma rexledesma left a comment

Choose a reason for hiding this comment

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

requesting changes for ^

@OwenKephart OwenKephart force-pushed the 08-02-_dagster-dbt_DA_Add_get_automation_condition_method_to_DagsterDbtTranslator branch from 5047565 to f2ff0a3 Compare August 8, 2024 18:01
@OwenKephart
Copy link
Contributor Author

@rexledesma I was keeping the get_auto_materialize_policy method because we didn't want to introduce truly breaking changes to AMP in the 1.8 release (that removal will happen in 1.9). Figured I'd keep the tests around to make sure that this change didn't break any existing users bc of that -- seem reasonable or is there a more aggressive thing you'd like to see here?

@OwenKephart OwenKephart requested a review from rexledesma August 8, 2024 18:03
Copy link
Contributor

@rexledesma rexledesma left a comment

Choose a reason for hiding this comment

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

got it, makes sense. we can wait to deprecate the other methods once AutomationCondition becomes GA in 1.9.

@OwenKephart OwenKephart merged commit 4d210a7 into master Aug 13, 2024
1 of 2 checks passed
@OwenKephart OwenKephart deleted the 08-02-_dagster-dbt_DA_Add_get_automation_condition_method_to_DagsterDbtTranslator branch August 13, 2024 16:09
PedramNavid pushed a commit that referenced this pull request Aug 14, 2024
…anslator (#23389)

## Summary & Motivation

Adds a `get_automation_condition` method to DagsterDbtTranslator, and
updates some docs references.

## How I Tested These Changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: docs Related to documentation in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants