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

Adds report node type #136

Merged
merged 4 commits into from
Sep 16, 2020
Merged

Adds report node type #136

merged 4 commits into from
Sep 16, 2020

Conversation

drewbanin
Copy link
Contributor

@drewbanin drewbanin commented Sep 15, 2020

resolves #135

Description

This PR adds:

  • A templated out details page for Report nodes
  • New node entries for Report node types

Additionally, this PR:

  • Cleans up "depends on" and "referenced by" UIs for things that are either roots or leafs (eg. sources cannot have parents, analyses cannot have dependents)
  • Fixes a bug where all entries showed "No resources reference this macro" for things that are not macros
  • Fixes quirks with navigation that prevented the DAG view from opening when a link to certain node types was navigated to

I can split this single PR into a couple of different smaller PRs if desired. This all needs to get vendored back into Core though, so I figured it was ok :)

Some specific feedback I'm looking for, though all is welcomed!

  • I added Reports as their own section under the Project nav. Do we like it there?
  • I picked orange for the DAG node color.... don't think that i love it, but there's certainly space to change that pretty easily!
  • Do we like the way info is being rendered in the table details? I think it's imperfect, but maybe passable for the time being.

TODO before merge:

  • Get some reviews
  • Update the changelog
  • Possibly split this out into multiple PRs if need be

Checklist

  • I have signed the CLA
  • I have generated docs locally, and this change appears to resolve the stated issue
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label Sep 15, 2020
@drewbanin drewbanin requested a review from jtcohen6 September 15, 2020 03:47
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Psyched about this! Thank you for picking it up in such short order!

I added Reports as their own section under the Project nav. Do we like it there?

I had the same thought, and I think this is right. I really like organizing these by type, too. The obvious question is around projects that may someday have dozens of these, since there isn't an additional mechanism for subfoldering. Type > maturity (high/medium/low/unknown)?

I picked orange for the DAG node color.... don't think that i love it, but there's certainly space to change that pretty easily!

Funny. For no reason in particular, I was thinking goldenrod—my favorite Pokémon city—so not far off.

Do we like the way info is being rendered in the table details? I think it's imperfect, but maybe passable for the time being.

More than passable! Two small things:

  • Is it possible to remove TAGS for now? I know that's more or less built in to every node type, it's just that reports cannot be tagged—yet, someday they probably should be.
  • I really like the "View this report" button! Not sure I'm sold on prepending the link to the description in addition

Last note: could you add a report or two to the ci-project + recompile artifacts, so that they'll show up in PR previews?

@drewbanin
Copy link
Contributor Author

@jtcohen6 the latest commit:

  • hides tags from report nodes
  • hides report configurations that are unset (eg. if no owner email is set, that table detail is not rendered)
  • removes the report link in the report description

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for those updates!

Only small thing is that while I see the updated manifest.json, I don't see the associated changes to ci-project to add the two reports

@drewbanin drewbanin merged commit 01c6ce2 into master Sep 16, 2020
@drewbanin drewbanin deleted the feature/report-nodes branch September 16, 2020 16:46
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.

Initial support for reports
2 participants