Enable test project hooks, flags.WHICH

[jtcohen6]

resolves #3463

Previously

• dbt test (and only dbt test) would not run on-run-start and on-run-end hooks.

This made sense in a time when test didn't actually create objects in the database, and the main motivation of on-run-end hooks was to run grant statementsm or other object/schema-level resource maintenance.

This carved-out exception has proven painful, though, because the {{ results }} context object, available only in the on-run-end context, contains a lot of valuable metadata (e.g. set of all test statuses/failures) that folks want to log, drop in the database, etc. (See: slack thread)

Change

• All "building" tasks should run these hooks: run, test, seed, snapshot, i.e. the tasks included by build. (Still excluded: compile, docs generate, list, source freshness, etc.)
• A new flag, WHICH, that gives users a way to run a specific hook (or not) depending on the current ask

{% macro my_test_end_hook() %}

  {% do log(flags.WHICH, info = true) %}
  {% if flags.WHICH == 'test' %}
    -- do something test-specific with {{ results }}
  {% else %}
    select 1 as id
  {% endif %}

{% endmacro %}

# dbt_project.yml
on-run-end:
  - "{{ my_test_end_hook() }}"

Commentary & questions

• I didn't previously have this on our v1.0 list, but it's a pretty simple and significant change, and I think it's one we should make. I also think it's less critical than before, with the advent of dbt build as the task to do it all.
• It feels a little uncomfortable that we expose the entire dbt.core.flags module to the Jinja context! Would we be better off exposing just a key-value dict containing all user-exposed flags/args?
• It would be very easy to support Add a "full run" indicator to Jinja context #2253 (comment) as well by adding flags.SELECT, flags.EXCLUDE, flags.SELECTOR. Or should we instead construct a generic args object that has it all?
• We'll need to document really clearly that all of these flags values are expected to change at execution time, in any invocation, so users must not use these as inputs to parse-time values (ref/source + configs) with partial parsing enabled—otherwise bad things will happen. It's also just not a good pattern! In my dream of dreams, a world where we've truly separate parsing from execution, these flags should return None at parse time. Is that a worthy thing to aspire for?

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 next" section.

[jtcohen6]

@gshank I'd appreciate your thoughts here, given your recent work on the flags module, and around parsing more generally. This isn't something we definitely need to do, but if we're going to do it, the change to the test task in particular feels like one we should make ahead of v1.0.

[gshank]

I agree that exporting the entire flags module feels wrong. We probably want to make a cleaner break between things that we support people looking up in jinja and things that are just for internal use. Also might want to prevent people from setting some of them directly. There is a dictionary available from get_flag_dict, but it doesn't have the subcommand flags in it.

The args object has gotten to act like a global setting store, but I'm not certain how I feel about that. We've expanded its use as a cli arg store quite past the original purpose, but it's just a Namespace class at the moment, and there's no behavior associated with it. It might be better to create an internal Settings object and copy the relevant args into it. We could have a read-only dictionary version of allowed settings available to jinja?

The flags/args/settings discussion aside, enabling this for tests seems like a good 1.0 feature.

[jtcohen6]

It might be better to create an internal Settings object and copy the relevant args into it. We could have a read-only dictionary version of allowed settings available to jinja?

@gshank I strongly agree with this. I'd be very comfortable with explicitly enumerating exactly which args/flags/settings should be accessible to the Jinja context. How tricky do you think this might be to implement?

Separately, would there be a way that we could offer backwards compatibility for folks who are used to reading a specific flag value into the Jinja context as flags.FLAG? Perhaps substituting a limited module that just redirects to the read-only dictionary? If that has to be a breaking change without backwards compatibility, it's not the end of the world; I think our built-in macros are the main consumers of flags.

[gshank]

I think there would be a bunch of trivial changes, so a fair number of lines of code to change, but not hard and mostly greppable. I'm thinking we'd replace internal use of flags entirely with a Settings object (probably a global one, kind of like the current adapter) and leave the flags module for compatibility for users. We kind of overuse the current args in a number of places so it's possible that would be a bit tricky to tease apart. The RPC server is going to be changing soon, so we might want to consult that team about what would work for them too.

[jtcohen6]

I left it as a TODO to rework the flags module, and expose an object that provides read-only access to users in the Jinja context. That's something we could hit if we have time ahead of v1.