-
Notifications
You must be signed in to change notification settings - Fork 67
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
Include DBT DRY RUN as a utilities plugin #1164
Conversation
👷 Deploy request for meltano-hub pending review.Visit the deploys page to approve it
|
Because this plugin and the underlying library is currently BigQuery only, I believe, what about using a name like |
@SBurwash thanks for the PR! To fix the CI tests you'll need to remove entity_url and |
@SBurwash in addition to the other suggestions, what do you think of having the image be the dbt logo with the words "Dry Run" over it? Or even the BQ logo as well? I don't think just the checkmark would look good on the Hub currently. |
@tayloramurphy I'm super happy with that! I didn't like my logo to begin with, I just have no talent at designing logos |
- testing | ||
- continuous-integrations | ||
label: dbt-dry-run | ||
logo_url: /assets/logos/utilities/dbt-dry-run.png |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SBurwash - Following up from @tayloramurphy's comment ... if it's easier than adding a new logo, you are also welcome to reuse this existing dbt logo if you'd like - or anything else in that same directory.
logo_url: /assets/logos/utilities/dbt.png |
This is consistent (today at least) with all the dbt flavors using the same logo:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SBurwash I mocked this up - what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks @tayloramurphy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SBurwash np! If you could just add this to the utility folder and update the yaml to reference it please 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍 PRETTY! 😄
@pnadolny13 I'm having some issues with my python version for the linter. Is this new version acceptable? I'm not sure what's bad order and what's improper values in the linter tests 😅 |
Co-authored-by: Pat Nadolny <patnadolny@gmail.com>
Co-authored-by: Pat Nadolny <patnadolny@gmail.com>
@SBurwash we have an open issue around either disabling or making the linting step easier. I'd say once you have your new logo updated we can force merge this and I'll update the linting separately. @tayloramurphy does that work? |
@pnadolny13 works for me! |
Tangental, but sharing here because it may be relevant to folks on this thread: I've logged a discussion in dbt-core around dry-run features in general - as discussed today in office hours. Feel free to watch/upvote/comment if interesting: |
label: dbt-dry-run | ||
logo_url: /assets/logos/utilities/dbt.png | ||
maintenance_status: active | ||
name: dbt-dry-dryn | ||
namespace: dbt-dry-run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
label: dbt-dry-run | |
logo_url: /assets/logos/utilities/dbt.png | |
maintenance_status: active | |
name: dbt-dry-dryn | |
namespace: dbt-dry-run | |
label: dbt-bigquery-dry-run | |
logo_url: /assets/logos/utilities/dbt.png | |
maintenance_status: active | |
name: dbt-bigquery-dry-dryn | |
namespace: dbt_bigquery_dry_run |
As discussed in office hours, proposing we add bigquery-
into the name where it will be presented in the Hub. This doesn't require changing the underlying repo but we should rename the file and path to match something like .../utilities/dbt-bigquery-dry-run/{variant_name}.yml
where variant name might be autotraderuk
or something else...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronsteers I wonder what you think about updating the label to something like dbt-dry-run (BigQuery Only)
and leaving the name and everything else the same as the repo/package name. It always feels a little funny to me to rename packages, although I know we do it for pipelinewise and airbyte so there is precedence for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pnadolny13 re:
@aaronsteers I wonder what you think about updating the label to something like
dbt-dry-run (BigQuery Only)
and leaving the name and everything else the same as the repo/package name.
That could work. Do I remember correctly that label
is used in search results and as the page title?
Presumably then, if a dbt-dry-run
were created for Snowflake, a key difference would be in whether names of future variants collide when adding side-by-side to Meltano. But this is just speculation since we don't know if/when non-BigQuery implementations will come about... 🤔
But anyway, yeah, I think I'm fine with adding ' (BigQuery)'
or ' (BigQuery Only)'
to the label if that accomplishes what we need to regarding discoverability and expectation-setting for new users.
@SBurwash I almost forgot - you should use the repo maintainer as the variant name and set it in the default_variants.yml. Like |
@pnadolny13 @aaronsteers thanks for the feedback! All requested changes should have been applied, please let me know if you would like me to do anything else! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SBurwash I fixed one quick typo. Looks good to me!
🚀 |
All Singer definitions are stored in
/_data/taps/
or/_data/targets
. The minimal requirement for adding a tap or target will match the following format:Checklist
/taps
or/targets
). The name of the file should match the name of the tap. If there is already one, add a descriptor to the name such as-search
./assets/logos/<taps or targets>
. The image name must match the YAML file name.@tayloramurphy
or@pnadolny13
to flag it for review. Or post to the #hub channel on Meltano slack.Reviewer Checklist