Add disabling variables for issue_assignee, issue_label, label, and requested_reviewer_history#64
Conversation
issue_assignee, issue_label, label, and requested_reviewer_history
fivetran-joemarkiewicz
left a comment
There was a problem hiding this comment.
A few comments and requests before approval
| creator.name as creator_name, | ||
| creator.company as creator_company, | ||
|
|
||
| {%- if var('github__using_requested_reviewer_history', True) -%} |
There was a problem hiding this comment.
I know it's a bit out of scope of this task, but can you please add the fully qualified name of these fields. Some fields have the fully qualified name (eg. issue_assignees.assignees) whereas others do not (hours_request_review_to_first_review). It makes it difficult to trace back where these fields are coming from.
No need to make this update across all the files, but for the ones being updated please make this update to make it easier to maintain in the future.
| hours_request_review_to_first_review, | ||
| hours_request_review_to_first_action, | ||
| hours_request_review_to_merge, |
There was a problem hiding this comment.
For fields like this that are no longer going to persist to the end model if the variables are disabled, we should updated the documentation accordingly to highlight that these fields will only be present with certain variables enabled.
There was a problem hiding this comment.
Updated and regenerated docs
packages.yml
Outdated
| # - package: fivetran/github_source | ||
| # version: [">=0.9.0", "<0.10.0"] | ||
| - git: https://github.com/fivetran/dbt_github_source.git | ||
| revision: feature/add-more-table-vars No newline at end of file |
There was a problem hiding this comment.
Reminder to swap before merge
.buildkite/scripts/run_models.sh
Outdated
| dbt deps | ||
| dbt seed --target "$db" --full-refresh | ||
| dbt source freshness --target "$db" || echo "...Only verifying freshness runs…" | ||
| dbt run --vars '{github__using_repo_team: false, github__using_issue_assignee: false, github__using_issue_label: false, github__using_requested_reviewer_history: false}' --target "$db" --full-refresh |
There was a problem hiding this comment.
| dbt run --vars '{github__using_repo_team: false, github__using_issue_assignee: false, github__using_issue_label: false, github__using_requested_reviewer_history: false}' --target "$db" --full-refresh | |
| dbt run --vars '{github__using_repo_team: false, github__using_issue_assignee: false, github__using_issue_label: false, github__using_requested_reviewer_history: false, github__using_label: false}' --target "$db" --full-refresh |
There was a problem hiding this comment.
Great catch -- updated myself directly
.buildkite/scripts/run_models.sh
Outdated
| dbt seed --target "$db" --full-refresh | ||
| dbt source freshness --target "$db" || echo "...Only verifying freshness runs…" | ||
| dbt run --vars '{github__using_repo_team: false, github__using_issue_assignee: false, github__using_issue_label: false, github__using_requested_reviewer_history: false}' --target "$db" --full-refresh | ||
| dbt test --vars '{github__using_repo_team: false, github__using_issue_assignee: false, github__using_issue_label: false, github__using_requested_reviewer_history: false}' --target "$db" |
There was a problem hiding this comment.
| dbt test --vars '{github__using_repo_team: false, github__using_issue_assignee: false, github__using_issue_label: false, github__using_requested_reviewer_history: false}' --target "$db" | |
| dbt test --vars '{github__using_repo_team: false, github__using_issue_assignee: false, github__using_issue_label: false, github__using_requested_reviewer_history: false, github__using_label: false}' --target "$db" |
There was a problem hiding this comment.
Updated myself directly
| github__using_issue_assignee: false # by default this is assumed to be true | ||
| github__using_issue_label: false # by default this is assumed to be true | ||
| github__using_label: false # by default this is assumed to be true | ||
| github__using_requested_reviewer_history: false # by default this is assumed to be true |
There was a problem hiding this comment.
Should we update line 84 with the similar tables not being synced?
fivetran-avinash
left a comment
There was a problem hiding this comment.
@fivetran-jamie Few comments before approval
fivetran-avinash
left a comment
There was a problem hiding this comment.
@fivetran-jamie Approved!
PR Overview
Package version introduced in this PR:
v0.9.1
This PR addresses the following Issue/Feature(s):
Summary of changes:
Creates new variables to disable the
issue_assignee,issue_label,label, andrequested_reviewer_historymodels if needed. Appropriately creates null fields or totally excludes fields/joins if the tables are missing.Submission Checklist
Consistency tests added for all models, passing
I'll share more detailed screenshots in Height, but without configuring the variables (default value = True)

dbt runproduces 43 modelsWith the variables set to False,

dbt runproduces 32 models8 of these are from the staging layer, as highlighted in the source PR
The remaining 3 are
int_github__issue_assignees,int_github__issue_labels, andint_github__issue_label_joinedChangelog