Add clearer directions for custom test suite vars in Makefile.

[VersusFacit]

resolves #6689

Description

The makefile is being used by community members after all! Hooray! Except, it's unclear how to use it. Booo.

Fixing that up:

• more explicit comments
• a dedicated spot in the Makefile for users to add "overrides"

History

I remember now! We did it this way because at the time it was super unclear how to inline an if-else in 1976's GNU Make. Well, I found a jacked up way to do it with filter!

Checklist

• I have read the contributing guide and understand what's expected of me
• 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 opened an issue to add/update docs, or docs changes are not required/relevant for this PR
• I have run changie new to create a changelog entry

[github-actions]

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

[VersusFacit]

For testing this, I made sure with echo that the command was being rendered properly.

1. make integration
2. USE_DEFAULT_FLAGS=true make integration
3. USE_DEFAULT_FLAGS=false make integration

@dsillman2000, do take a look and let me know if you dis/like this behavior and whether the comments are now un/clear.

[VersusFacit]

Additional test case since I realize I should show how the new system works when you've uncommented the custom ci flags var in the Makefile and passed the use flag as false (or anything other than true really).

[dbeatty10]

Today I Learned

Reading here, it looks like the make command allows for overriding variables!

Consider a Makefile defined like:

DBT_TEST_USER_1 := "dbt_test_user_1"
DBT_TEST_USER_2 := "dbt_test_user_2"
DBT_TEST_USER_3 := "dbt_test_user_3"
RUSTFLAGS := "-D warnings"
LOG_DIR := "./logs"
DBT_LOG_FORMAT := "json"

CI_FLAGS =\
	DBT_TEST_USER_1=$(DBT_TEST_USER_1)\
	DBT_TEST_USER_2=$(DBT_TEST_USER_2)\
	DBT_TEST_USER_3=$(DBT_TEST_USER_3)\
	RUSTFLAGS=$(RUSTFLAGS)\
	LOG_DIR=$(LOG_DIR)\
	DBT_LOG_FORMAT=$(DBT_LOG_FORMAT)

test:
	@echo $(CI_FLAGS)

Then run:

make test

The output is:

DBT_TEST_USER_1=dbt_test_user_1 DBT_TEST_USER_2=dbt_test_user_2 DBT_TEST_USER_3=dbt_test_user_3 RUSTFLAGS=-D warnings LOG_DIR=./logs DBT_LOG_FORMAT=json

Then run this instead:

make test LOG_DIR=/tmp DBT_LOG_FORMAT=text

The output changes accordingly:

DBT_TEST_USER_1=dbt_test_user_1 DBT_TEST_USER_2=dbt_test_user_2 DBT_TEST_USER_3=dbt_test_user_3 RUSTFLAGS=-D warnings LOG_DIR=/tmp DBT_LOG_FORMAT=text

Suggestion

If we use the variable definitions from the Makefile above, then we can just use the following in both places where it is relevant (without if statements or a filter or a new USE_DEFAULT_CI_FLAGS variable):

$(CI_FLAGS) $(DOCKER_CMD) tox -e py-integration -- -nauto

This would allow the user to override any variables when desired without needing to make any edits within the Makefile.

Interested to hear your feedback on this suggestion!

[VersusFacit]

Yeah, that's one (of 4 lol) way(s) to do it 😉 With so many though, and thinking how unix environment variable overriding might not be the most intuitive thing, that dbt users might prefer a way to just load up their Makefile and forget about it rather than reconstructing the massive env_var string on the command line every time (or digging through history logs, or writing their own shortcut scripts, or whatever). 🤷‍♀️ Like what you said is true, and I wish I would have said in the PR originally that I had considered this.

But suffice to say, my guess was that wasn't the right solution for here since we like to template out everything for users.

If we trust that anyone using the Makefile is a power user who can be bothered to futz around with Bash and write their own scripts/aliases (not sure I'm confident about that), then sure, I'd prefer what you wrote.

Also what you wrote uses := which is fine, but we'd have to explain to people how that works since the standard is to set env vars ahead of the executable; which requires use of the ?= operator.

tl;dr let's decide upon a use of syntax that is consistent across all tooling and whether we should encourage people to edit Makefile tooling or not.

[VersusFacit]

It probably makes sense not to encourage editing the Makefile on second thought, but I'm not convinced what you're suggestion is really the best solution for our users. Let me go back to the drawing board on this for just a sec. Something I want to try.

[VersusFacit]

Collapsed this variable into the LOG_DIR variable used in our CI

[VersusFacit]

Alright @dbeatty10 what about this scheme? I have to edit a few things, but the target behavior looks good.

Trying for best of all worlds, no make expertise required to override variables; just make a makefile.text.env as detailed in the CONTRIBUTING.md. I think this is how we prefer across all tooling repos to let users override environment values.

When it's empty or not made, here's the result of CI_FLAGS:

With an override:

Also, don't mind missing quotes. They are treated as a single string in bash/python jobs. No I don't understand why make insists displaying values this way 😭

[stu-k]

One nonblocking comment on the contributing doc, otherwise nothing stands out. I'd recommend getting a 👍 from someone other than myself before merging as I am not familiar with our Makefile or CI flags.

[VersusFacit]

Doug and I decided for now it's better to just have this note here, since anyone interested in overriding the Makefile would be the type of person who needs this information. Better not to clutter the CONTRIBUTING.md with very specific information.

[dbeatty10]

🚀 Thanks for your work on this @VersusFacit !