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

Maja dbt certification #23

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Maja dbt certification #23

wants to merge 8 commits into from

Conversation

MajaCernja
Copy link

No description provided.

@MajaCernja MajaCernja requested a review from evb123 January 2, 2024 10:28
@evb123
Copy link

evb123 commented Jan 2, 2024

Looks great! Some comments/actions:

  • why did you upgrade versions (do they need to be upgraded)?
  • the sensitive data handling is done in the 'old' way - do you know what things you need to change and add to conform to the current guidance?
  • Some of you pull request does not match our current guidance - see here for more deets.
  • Notice anything out of date/unclear in the certification or this part? Let me know so I can fix or make a PR yourself :)

@MajaCernja
Copy link
Author

Thank you @evb123!

  • Re: upgraded versions, I just remember encountering an error that suggested I should update the packages so I went ahead with it, in general should I avoid making updates like this?
  • For sensitive data I believe the separation into a sensitive folder is no longer necessary and I should add a .sqlfluffignore with the staging file using hashed sensitive columns
  • Noted, will address!
  • The guidance on sensitive data in the Readme of this repo seems to be out of date (it's why I went the route of creating the sensitive folder), I can amend that as I work on my PR

@MajaCernja MajaCernja force-pushed the maja-dbt-certification branch from 6857d1e to 2cf87a8 Compare January 3, 2024 11:13
@evb123
Copy link

evb123 commented Jan 3, 2024

Great! Also thanks for sensitive in README - I wasn't aware it was there. Some answers:

  • Okay, perfect, I was just curious.
  • Yes correct- theres also one thing missing in the _models.yml entry - the _pii view is not tagged as sensitive - it needs this to be added to consumer_sensitive (currently dbt doc police should pick up _pii views that try to get written to consumer)
  • Specifically: PR descriptions are always needed, commit messages should begin with upper case and imperative verb -> 'warehouse refactor' would be 'Refactor warehouse'
  • Great! Thanks so much!!

Its okay to not get your PR to pass - I reckon the circle-ci checks are out of date and we need to update it.

@evb123
Copy link

evb123 commented Jan 3, 2024

Notes: Project evaluator tests seem to be failing on hash macro - not sure how to exclude

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

Successfully merging this pull request may close these issues.

2 participants