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.

Sign up for GitHub

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

Jump to bottom

[CT-1101] 024_custom_schema_tests #5828

Merged
merged 5 commits into from
Sep 20, 2022
Merged

[CT-1101] 024_custom_schema_tests #5828

merged 5 commits into from
Sep 20, 2022

Conversation

colin-rogers-dbt
Copy link
Contributor

@colin-rogers-dbt colin-rogers-dbt commented Sep 13, 2022 •
edited
Loading

resolves #5732

Description

Checklist

@colin-rogers-dbt colin-rogers-dbt requested a review from a team as a code owner September 13, 2022 16:26
@cla-bot cla-bot bot added the cla:yes label Sep 13, 2022
@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.

@colin-rogers-dbt colin-rogers-dbt changed the title delete old integration tests [CT-1101] 024_custom_schema_tests Sep 13, 2022
@McKnight-42
Copy link
Contributor

@colin-rogers-dbt this is looking fantastic! a few small things I saw are that for test conversions we typically actually skip the changelog entry (as we see it as just a adjustment of current functionality), and that there isn't currently a PR for the dbt-snowflake version of this test.

iknox-fa
iknox-fa reviewed Sep 19, 2022
View reviewed changes
tests/functional/schema/test_custom_schema.py Outdated
):
project.run_sql(_VALIDATION_SQL)
run_dbt(["seed"])
# run_dbt(["build"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this was left in as a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mostly cause I forgot to delete it

@colin-rogers-dbt
Copy link
Contributor Author

@colin-rogers-dbt this is looking fantastic! a few small things I saw are that for test conversions we typically actually skip the changelog entry (as we see it as just a adjustment of current functionality)

makes sense, should I just delete the entry?

there isn't currently a PR for the dbt-snowflake version of this test.

Will raise a pr there to delete/migrate the tests in dbt-snowflake, do I need to check the other adapters as well?

iknox-fa
iknox-fa approved these changes Sep 19, 2022
View reviewed changes
Copy link
Contributor

@iknox-fa iknox-fa left a comment

Choose a reason for hiding this comment

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

One errant comment, otherwise LGTM

@McKnight-42
Copy link
Contributor

yes should just be able to delete the entry then apply the skip changelog label to your pr so it skips that check

and I believe snowflake is the only other adapter to have a modified version of this test but yeah double checking is always good! 👍

@colin-rogers-dbt colin-rogers-dbt added the Skip Changelog Skips GHA to check for changelog file label Sep 19, 2022
@colin-rogers-dbt colin-rogers-dbt merged commit 0bd6df0 into main Sep 20, 2022
@colin-rogers-dbt colin-rogers-dbt deleted the convert_024_custom_schema_tests branch September 20, 2022 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iknox-fa iknox-fa iknox-fa approved these changes

@nathaniel-may nathaniel-may Awaiting requested review from nathaniel-may nathaniel-may was automatically assigned from dbt-labs/core-group

@McKnight-42 McKnight-42 Awaiting requested review from McKnight-42 McKnight-42 was automatically assigned from dbt-labs/core-group

Assignees
No one assigned
Labels
cla:yes Skip Changelog Skips GHA to check for changelog file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CT-1101] 024_custom_schema_tests
3 participants
@colin-rogers-dbt @McKnight-42 @iknox-fa