[CT-1874] [Bug] Erroneous Makefile variable causing make integration to fail

[dsillman2000]

Is this a new bug in dbt-core?

• I believe this is a new bug in dbt-core
• I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

My friend and coworker @seub has followed the Contributing guide for setting up the development environment for DBT-core, but repeatedly failed the make integration pytest suite. Many of the error messages referenced the fact that some environment variables weren't defined (the environment variables provided in the $CI_FLAGS variable). Looking into lines 66-69 of the makefile, there's this conditional statement contributed by @VersusFacit one or two commits ago:

.PHONY: integration
integration: .env ## Runs postgres integration tests with py-integration
	@\
	$(if $(USE_CI_FLAGS), $(CI_FLAGS)) $(DOCKER_CMD) tox -e py-integration -- -nauto

Because the variable $USE_CI_FLAGS is not defined anywhere, the conditional fails and the CI_FLAGS are not provided to the DOCKER_CMD. After removing the if statement and simply providing the $CI_FLAGS unconditionally, make integration succeeded.

Expected Behavior

I expected the make integration test to succeed after following the Contributing guide.

Steps To Reproduce

Create a fresh dev environment for dbt-core and try running make integration off the main branch.

Relevant log output

No response

Environment

- OS: macOS 12.2 (M1)
- Python: 3.9.10

Which database adapter are you using with dbt?

No response

Additional Context

Suggested solutions:

1. Remove the conditional from the Makefile (unconditionally providing the CI_FLAGS), line 69 becomes:

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

2. Update the contributing guide to use a different make tag than integration, at least until this issue is resolved.

[dsillman2000]

UPDATE: Through communications in the dbt-developer slack, it's been revealed that the Makefile works as expected if the environment variable USE_CI_FLAGS is set, but this variable is not mentioned anywhere in the Contributing guide. If it's expected that users set this environment variable when testing dbt-core, they should be expressly instructed to do so in the Contributing guide.

So, consider potential solution number 3 to be "update the contributing guide to tell the user to set the USE_CI_FLAGS environment variable to true before running make integration.

[VersusFacit]

Yeah we should probably go with solution three to explain what this is doing. I can only barely remember the context of this right now. That said, let me do some digging and recall so we can make a better experience here.

Also, I do vaguely remember there being a PR out there for this solution as opposed to another. I have to go digging.

Thanks for the flag! Tooling is only as good as it can be understood.

[VersusFacit]

Alrightie ⌛ Had some time to dig through the commit log/PR history.

I like the spirit of this solution because ultimately it's trying to preserve flexibility of the tooling: users may have environments with different environment variable values.

That said, with wiser eyes today, I'd like a revise. How would we feel about making this default_ci_flags or somesuch; then, make this variable included by default and comment what is happening in the Makefile or contributing page that a user should override this as they see fit for their environment?

edit: just a historical perspective, I think the team as a whole doesn't really have visibility into who uses this tooling (we do ourselves, of course!!). Since we rarely hear complaints, we rarely know what might be better ways to improve this. All in all, a great little bit of feedback you brought to us for the future 💥

[dbeatty10]

@VersusFacit

I like the direction you're suggesting:

1. make all the relevant environment variables included by default (possibly through a variable named default_ci_flags)
2. include comments in the contributing page (or Makefile) of what a user should override if needed

[VersusFacit]

PR with some adjustments is up. Happy to iterate the comments, so get your opinions on there.