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

Rename "constraints_enabled" to "contract" #7002

Merged
merged 2 commits into from
Feb 23, 2023

Conversation

gshank
Copy link
Contributor

@gshank gshank commented Feb 17, 2023

resolves #6748

Description

After more discussion and considering the scope of this setting, we decided to rename "constraints_enabled" to "contract".

Checklist

@gshank gshank requested review from a team as code owners February 17, 2023 15:54
@cla-bot cla-bot bot added the cla:yes label Feb 17, 2023
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@gshank gshank requested a review from MichelleArk February 17, 2023 16:06
@jtcohen6 jtcohen6 mentioned this pull request Feb 21, 2023
14 tasks
Copy link
Contributor

@MichelleArk MichelleArk left a comment

Choose a reason for hiding this comment

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

Changes here look good! There is some boilerplate in each adapter implementation that references config.get('constraints_enabled') (dbt-snowflake example), which will need to be updated as well. I can make corresponding issues in the adapter repos 👍

@MichelleArk
Copy link
Contributor

We should close the corresponding adapter issues prior to merging this to prevent breaking tests in adapters, since they inherit from functional tests modified in this PR.

@gshank
Copy link
Contributor Author

gshank commented Feb 23, 2023

We can't close the adapter issues prior to merging this because the "contract" config wouldn't actually exist in core until after this is merged. The has to merge first, then all of the adapter issues.

@gshank
Copy link
Contributor Author

gshank commented Feb 23, 2023

Actually, I have a question about this now that I think about it. We're "promoting" the "contract" attribute to the model level. Shouldn't we be accessing it via the model, like "model.contract", and not looking at the config? This is one of those fields where the config is just used to shuttle the value in node level...

@gshank
Copy link
Contributor Author

gshank commented Feb 23, 2023

So actually the config.get calls should still work in the adapter repos since it doesn't require that the config attribute actually work. But if we did model.contract they might fail.

@gshank gshank merged commit 58d1bcc into main Feb 23, 2023
@gshank gshank deleted the ct-1916-rename_constraints_enabled branch February 23, 2023 17:16
@sungchun12
Copy link
Contributor

@gshank thanks for evolving contracts by actually having the config called contract. Even that simple visual cue as I tested this out today was so much more intuitive than constraints_enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-1916] Rename 'constraints_enabled' to 'contract'
3 participants