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

[CT-789] Add Grants to Materializations #212 #131

Merged
merged 23 commits into from
Jul 11, 2022
Merged

Conversation

dbeatty10
Copy link
Contributor

@dbeatty10 dbeatty10 commented Jul 7, 2022

resolves #128

Description

Includes the tests in:

Todo before merge

  • Remove unused DBT_TEST_USER_* environment variables from secrets
  • Update dev-requirements.txt

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt-redshift next" section.

@dbeatty10 dbeatty10 closed this Jul 7, 2022
@dbeatty10 dbeatty10 reopened this Jul 7, 2022
@McKnight-42
Copy link
Contributor

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Jul 7, 2022

The cla-bot has been summoned, and re-checked this pull request!

@dbeatty10 dbeatty10 mentioned this pull request Jul 8, 2022
4 tasks
@dbeatty10 dbeatty10 marked this pull request as ready for review July 8, 2022 02:13
@dbeatty10 dbeatty10 marked this pull request as draft July 8, 2022 02:13
Comment on lines 9 to 17
my_snapshot_sql = """
{% snapshot my_snapshot %}
{{ config(
check_cols='all', unique_key='id', strategy='check',
target_database=database, target_schema=schema
) }}
select 1 as id, cast('blue' as {{ type_string() }}) as color
{% endsnapshot %}
""".strip()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this override, I was getting this error upon the 2nd of 2 consecutive snapshots:

failed to find conversion function from "unknown" to text

Root cause

Old versions of Postgres (and apparently current versions of Redshift) have a known issue with detecting types of untyped constants [1][2][3]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Optionally, we could add this (slightly) more verbose version to dbt-core.

On that note, is there syntax for type-casting that we could enable for core + adapters?

I'd prefer less verbose syntax like this:

select 1 as id, {{ cast_string('blue') }} as color

over this:

select 1 as id, cast('blue' as {{ type_string() }}) as color

The former would also allow some adapters to render more pithy SQL like:

'blue'::text

rather than:

cast('blue' as text)

Copy link
Contributor

Choose a reason for hiding this comment

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

@dbeatty10 I think we could add a cross-db {{ cast() }} macro to dbt-core, which returns column::type where it's supported, or cast(column as type) otherwise / by default.

For now, I did the simpler version in dbt-labs/dbt-core@7f499ba

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, I'll try the relevant update and see if it works.

@dbeatty10 dbeatty10 requested a review from jtcohen6 July 11, 2022 01:25
@dbeatty10 dbeatty10 marked this pull request as ready for review July 11, 2022 01:26
@jtcohen6
Copy link
Contributor

Rerunning CI to account for changes in dbt-labs/dbt-core@c763601. I don't believe any code changes here are needed.

Copy link
Contributor

@McKnight-42 McKnight-42 left a comment

Choose a reason for hiding this comment

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

Great work @dbeatty10

@dbeatty10 dbeatty10 merged commit 9a3492a into main Jul 11, 2022
@dbeatty10 dbeatty10 deleted the dbeatty/ct-789-grant-sql branch July 11, 2022 17:49
abbywh pushed a commit to abbywh/dbt-redshift that referenced this pull request Oct 11, 2023
* Use development branch of dbt-core [skip ci]

* Adapter-specific macro to show grants / get a list of table privileges from Redshift

* Replace with tests that exist

* Inherit functional tests for grants

* Align `privilege_type` nomenclature with postgres adapter

* Add new environment variables to CI

* Add new environment variables to CI

* Try hard-coded user names

* Remove adapter-specific implementation of `get_revoke_sql`

* Covert hard-code env vars to pull from GitHub secrets instead

* Plain text rather than repo secrets

* Filter out grants to the current user

* Switch to branch with more tests [skip ci]

* Ignore super users

* Replace untyped constant with an explicit typed constant

* Remove extraneous `pass`

* Update CHANGELOG [skip ci]

* Fix CHANGELOG [skip ci]

* Inherit default tests

* Update development branch

* Point back to main branch for dbt-core
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-789] Add support for grant macros
3 participants