Feature/sunset full statement version #109

fivetran-catfritz · 2024-06-27T21:29:40Z

PR Overview

This PR will address the following Issue/Feature:

[Feature] fully sunset full_statement_version=false code #105

This PR will result in the following new package version:

v0.10.0 breaking because of deprecation

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

🚨 Breaking Changes 🚨

The parameter full_statement_version=false has been fully sunset from the sfdc_formula_view macro. You will now need to remove this parameter to avoid a compilation error.

Under the hood

Included auto-releaser GitHub Actions workflow to automate future releases.

Updated maintainer pull request template.

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

dbt run –full-refresh && dbt test
~~dbt run (if incremental models are present) && dbt test~~

Before marking this PR as "ready for review" the following have been applied:

The appropriate issue has been linked, tagged, and properly assigned
All necessary documentation and version upgrades have been applied
docs were regenerated (unless this PR does not include any code or yml updates)
BuildKite integration tests are passing
Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

Consistency tests pass
When setting full_statement_version=false in a model, a compilation error is successfully thrown.

Additional notes

full_statement_version=true continues to be allowed so that users do not experience the below compilation failure since the macro cannot take an unexpected argument.

If you had to summarize this PR in an emoji, which would it be?

💃

fivetran-joemarkiewicz

@fivetran-catfritz thanks for addressing this update so quickly! I have a few comments and requests before approval.

While we are sunsetting functionality, I don't want a customer to upgrade and it possibly cause catastrophic failures in their dbt project. Ideally, we create "soft" failures and provide informative messages so customers know what actions they need to take if the release notes were not reviewed prior to upgrading.

Let me know if you want to sync on my comments and if you want to brainstorm some ideas. Thanks!

CHANGELOG.md

macros/sfdc_formula_view.sql

fivetran-catfritz

@fivetran-joemarkiewicz thank you for the review comments! I have made the updates, and this is ready for re-review.

CHANGELOG.md

macros/sfdc_formula_view.sql

fivetran-joemarkiewicz

@fivetran-catfritz great work on this PR! This is nearly ready, I just have a few final comments before this is ready for approval and release.

fivetran-joemarkiewicz · 2024-07-05T19:43:47Z

README.md

@@ -21,14 +21,15 @@ This package includes macros and scipts to be used within a dbt project to accur
 To use this dbt package, you must have the following:
 - At least one Fivetran Salesforce connector syncing data into your destination. 
 - A **BigQuery**, **Snowflake**, **Redshift**, **PostgreSQL**, or **Databricks** destination.
+- **dbt Core**: This package is designed to work with dbt Core and is **not** compatible with dbt Cloud.


The package is technically compatibile with dbt Core. It is only this section that is not. Could we remove this change and instead update this section to callout that it is only compatible with dbt Core.

Got it. Updated!

macros/sfdc_formula_view.sql

fivetran-joemarkiewicz · 2024-07-05T20:12:10Z

macros/sfdc_formula_view.sql

+-- Raise a warning if users are trying to use full_statement_version=false. We are keeping the variable in the macro, however, since we don't want errors if they previously set it to true.
+{% if not full_statement_version %}
+    {{ exceptions.warn("\ERROR: The full_statement_version=false, reserved_table_name, and fields_to_include parameters are no longer supported. Please update your " ~ this.identifier|upper ~ " model to remove these parameters.\n") }}
+    * See full model error in log.


In BigQuery this error is a bit confusion.

What if we do this instead?

Suggested change

* See full model error in log.

See_full_model_error_in_log

Which provides the following error.

I see. That works for me!

fivetran-catfritz

Thanks for the comments @fivetran-joemarkiewicz! I have made the updates, and it's ready for re-review!

fivetran-catfritz · 2024-07-08T22:29:39Z

README.md

@@ -21,14 +21,15 @@ This package includes macros and scipts to be used within a dbt project to accur
 To use this dbt package, you must have the following:
 - At least one Fivetran Salesforce connector syncing data into your destination. 
 - A **BigQuery**, **Snowflake**, **Redshift**, **PostgreSQL**, or **Databricks** destination.
+- **dbt Core**: This package is designed to work with dbt Core and is **not** compatible with dbt Cloud.


Got it. Updated!

macros/sfdc_formula_view.sql

fivetran-catfritz · 2024-07-08T22:33:41Z

macros/sfdc_formula_view.sql

+-- Raise a warning if users are trying to use full_statement_version=false. We are keeping the variable in the macro, however, since we don't want errors if they previously set it to true.
+{% if not full_statement_version %}
+    {{ exceptions.warn("\ERROR: The full_statement_version=false, reserved_table_name, and fields_to_include parameters are no longer supported. Please update your " ~ this.identifier|upper ~ " model to remove these parameters.\n") }}
+    * See full model error in log.


I see. That works for me!

fivetran-joemarkiewicz

@fivetran-catfritz just a few last comments. Let me know once ready for re-review.

fivetran-joemarkiewicz · 2024-07-09T14:27:43Z

macros/sfdc_formula_view.sql

@@ -1,4 +1,4 @@
-{%- macro sfdc_formula_view(source_table, source_name='salesforce', reserved_table_name=source_table, fields_to_include=none, full_statement_version=true, materialization='view', using_quoted_identifiers=False) -%}
+{%- macro sfdc_formula_view(source_table, source_name='salesforce', materialization='view', using_quoted_identifiers=False, full_statement_version=true) -%}


I was doing some final tests and noticed a major compilation error when testing what happens when I still have reserved_table_name or fields_to_include as a parameter. For example the following code:

{{ salesforce_formula_utils.sfdc_formula_view( source_table='task_priority', fields_to_include=['new_field','cool_field']) }}

Results in the following full dbt project level compilation error (nothing else in the project can compile).

This error completely ignores the informative error message you provide in the logs. I don't want to risk potentially breaking an entire workflow because we wanted to remove these parameters. I know I recommended removing these, but given this error message this is too dangerous. I recommend we keep all the terminology that these fields are deprecated (because they are and don't do anything).

Suggested change

{%- macro sfdc_formula_view(source_table, source_name='salesforce', materialization='view', using_quoted_identifiers=False, full_statement_version=true) -%}

{%- macro sfdc_formula_view(source_table, source_name='salesforce', reserved_table_name=source_table, fields_to_include=none, full_statement_version=true, materialization='view', using_quoted_identifiers=False) -%}

From our discussion, I added the defaults for the variables back in but keeping the conditions for the warning the same.

fivetran-catfritz

@fivetran-joemarkiewicz this is ready for re-review!

fivetran-catfritz · 2024-07-09T16:28:53Z

macros/sfdc_formula_view.sql

@@ -1,4 +1,4 @@
-{%- macro sfdc_formula_view(source_table, source_name='salesforce', reserved_table_name=source_table, fields_to_include=none, full_statement_version=true, materialization='view', using_quoted_identifiers=False) -%}
+{%- macro sfdc_formula_view(source_table, source_name='salesforce', materialization='view', using_quoted_identifiers=False, full_statement_version=true) -%}


From our discussion, I added the defaults for the variables back in but keeping the conditions for the warning the same.

fivetran-joemarkiewicz

LGTM Thanks for all your work on this PR @fivetran-catfritz I have just one final CHANGELOG request but that won't block the approval. Once that last CHANGELOG update is applied this will be good for release review.

fivetran-joemarkiewicz · 2024-07-09T17:16:52Z

CHANGELOG.md

+
+## 🚨 Breaking Changes 🚨
+- As announced in the [v0.9.2 August 2023 release](https://github.com/fivetran/dbt_salesforce_formula_utils/releases/tag/v0.9.2), the parameter `full_statement_version=false` has now been fully sunset from the `sfdc_formula_view` macro. You will now need to remove this parameter to avoid a compilation error.
+  - As part of this deprecation, the parameters `reserved_table_name` and `fields_to_include` have also been deprecated and will also need to be removed. 


Can you also include a sub bullet mentioning all of the macros that were removed as part of this update.

Good call. Updated!

fivetran-jamie

lgtm! just one suggestion to insert a link in one more place

.github/ISSUE_TEMPLATE/bug-report.yml

Co-authored-by: Jamie Rodriguez <[email protected]>

fivetran-catfritz added 2 commits June 27, 2024 14:13

feature/sunset-full-statement-version

f0011e6

add consistency tests

bfd0df2

fivetran-catfritz self-assigned this Jun 27, 2024

fivetran-catfritz added 3 commits June 27, 2024 16:31

add auto release and update pr template

4653833

add auto release and update pr template

88f8df0

update consistency tests

b2acd91

fivetran-catfritz requested a review from fivetran-joemarkiewicz June 28, 2024 15:29

fivetran-joemarkiewicz requested changes Jun 28, 2024

View reviewed changes

fivetran-catfritz added 3 commits July 1, 2024 18:56

revisions from review

9578cb3

update changelog and macro

072fb76

regen docs

aed79e3

fivetran-catfritz commented Jul 3, 2024

View reviewed changes

fivetran-catfritz requested a review from fivetran-joemarkiewicz July 3, 2024 23:00

update readme and issue templates

9e31eef

fivetran-joemarkiewicz requested changes Jul 5, 2024

View reviewed changes

updates from review

5bd8de9

fivetran-catfritz commented Jul 8, 2024

View reviewed changes

fivetran-joemarkiewicz requested changes Jul 9, 2024

View reviewed changes

update default vars

c7acfbd

fivetran-catfritz commented Jul 9, 2024

View reviewed changes

fivetran-catfritz requested a review from fivetran-joemarkiewicz July 9, 2024 16:48

fivetran-joemarkiewicz approved these changes Jul 9, 2024

View reviewed changes

update changelog

c4867ce

fivetran-jamie approved these changes Jul 9, 2024

View reviewed changes

.github/ISSUE_TEMPLATE/bug-report.yml Outdated Show resolved Hide resolved

Update .github/ISSUE_TEMPLATE/bug-report.yml

e38ac71

Co-authored-by: Jamie Rodriguez <[email protected]>

fivetran-catfritz merged commit c12dc44 into main Jul 9, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/sunset full statement version #109

Feature/sunset full statement version #109

fivetran-catfritz commented Jun 27, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-catfritz left a comment

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jul 5, 2024

fivetran-catfritz Jul 8, 2024

fivetran-joemarkiewicz Jul 5, 2024

fivetran-catfritz Jul 8, 2024

fivetran-catfritz left a comment

fivetran-catfritz Jul 8, 2024

fivetran-catfritz Jul 8, 2024

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jul 9, 2024

fivetran-catfritz Jul 9, 2024

fivetran-catfritz left a comment

fivetran-catfritz Jul 9, 2024

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jul 9, 2024

fivetran-catfritz Jul 9, 2024

fivetran-jamie left a comment

		@@ -1,4 +1,4 @@
		{%- macro sfdc_formula_view(source_table, source_name='salesforce', reserved_table_name=source_table, fields_to_include=none, full_statement_version=true, materialization='view', using_quoted_identifiers=False) -%}
		{%- macro sfdc_formula_view(source_table, source_name='salesforce', materialization='view', using_quoted_identifiers=False, full_statement_version=true) -%}

Feature/sunset full statement version #109

Feature/sunset full statement version #109

Conversation

fivetran-catfritz commented Jun 27, 2024 • edited Loading

PR Overview

🚨 Breaking Changes 🚨

Under the hood

PR Checklist

Basic Validation

Detailed Validation

Additional notes

If you had to summarize this PR in an emoji, which would it be?

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-catfritz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-catfritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-catfritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-jamie left a comment

Choose a reason for hiding this comment

fivetran-catfritz commented Jun 27, 2024 •

edited

Loading