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

[CT-756] Stringify user-provided messages to {{ log() }} #1

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DavidMah
Copy link
Owner

@DavidMah DavidMah commented Aug 20, 2022

draft: I'm using this rather randomly selected task to explore the workflow of contributing to dbt-core

TODO: get CLA through figma legal
TODO: post PR against actual repo
TODO: the changie thing


resolves dbt-labs#5385

Description

Much more depth in the issue dbt-labs#5385. This PR fixes an issue wherein log errors in the scenario that

  • User is using dbt env var secrets, which in turn enables the mechanism that scrubs secrets from log messages.
  • Call log with something other than a string, for instance: {{ log(["this is something other than a string"]) }}

This PR implements the solution articulated in the issue and throws in a test case too.

Checklist

@@ -43,7 +43,7 @@ def test__override_vars_global(self, project):


# This one switches to setting a var in 'test'
class TestCLIVarOverridePorject:
class TestCLIVarOverrideProject:
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is a completely unrelated change to fix this typo. I'm curious what the culture is about bundling in tiny unrelated changes like this. Is it fine or is it expected to be broken out to a separate PR?

try:
msg = str(msg)
except Exception as e:
raise RuntimeException("log failed to stringify the given object") from e
Copy link
Owner Author

Choose a reason for hiding this comment

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

Did a one-off test for this, since I can't think of a convenient way to induce this within the test case via jinja provided input (given dbt/jinja design, I'd sort of hope not 😆)

image

@@ -0,0 +1,55 @@
import pytest
Copy link
Owner Author

Choose a reason for hiding this comment

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

Not sure if this is even the appropriate variation of test for this part of the codebase, I found this directory through randomly wandering, and then I copied adjacent files to get it to work.

@@ -11,6 +11,7 @@
from dbt.exceptions import (
CompilationException,
MacroReturn,
RuntimeException,
Copy link
Owner Author

Choose a reason for hiding this comment

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

Is there a more specific/appropriate exception than RuntimeException?

@@ -543,10 +544,11 @@ def zip_strict(*args: Iterable[Any]) -> Iterable[Any]:

@contextmember
@staticmethod
def log(msg: str, info: bool = False) -> str:
def log(msg: Any, info: bool = False) -> str:
Copy link
Owner Author

Choose a reason for hiding this comment

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

@DavidMah DavidMah force-pushed the dmah/ct-756-log-str branch from d2bf298 to eef4cb0 Compare August 21, 2022 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-756] Stringify user-provided messages to {{ log() }}
2 participants