Chore(cicd_bot): Show logged warnings/errors in the check output by erindru · Pull Request #4938 · SQLMesh/sqlmesh

erindru · 2025-07-09T05:58:34Z

Prior to this PR, warnings were logged to the console only. This made them vastly less noticeable/actionable because you'd only ever see them if you were trying to troubleshoot an issue.

PR #4900 introduces a warning that we want people to take notice of / action.
So, this PR:

Refactors how the PR Environment Summary Markdown is generated as it was getting quite large and unwieldy
Update MarkdownConsole errors / warnings captured output to produce alert blocks instead of code blocks
Utilize these when writing the check output

Before (warnings/errors not shown in check output):

After (warnings collapsed if too long):

After (warnings expanded)

After (warnings truncated if too long)

After (error example)

themisvaltinos

lgtm the alert block is really nice there! one question I have is about the error alert and how it looks because I can't find examples of it on GitHub

themisvaltinos · 2025-07-09T21:31:24Z

sqlmesh/integrations/github/cicd/controller.py

                return f"No changes to apply.{plan_flags_section}"

-            return f"{difference_summary}\n{missing_dates}{plan_flags_section}"
+            warnings_block = self._console.captured_warnings


fmi what is the reason that here you don't consume so the error and warnings remain but in get_pr_environment_summary you use consume_captured_warnings instead

Good question.

I need to revisit this, upon further reflection it's essentially a no-op and because the warnings already get consumed in get_pr_environment_summary which means self._console.captured_warnings always returns ''

I've updated these to the consume() equivalents. Most of the time, warnings in particular will be already consumed by the PR Environment Synced step, this would just capture any extra ones raised between the steps

yes i see, it is indeed a bit hard to follow the steps and see what has been consumed

themisvaltinos · 2025-07-09T21:34:36Z

sqlmesh/core/console.py

-        if errors:
-            self._errors.append("\n".join(str(ex) for ex in errors))
-            super().log_failed_models(errors)
+        self._errors.extend([str(ex) for ex in errors])


should this have similar logic check like in log_error if message not in self._errors: or the errors can't have duplicates here

Yeah I was torn between preserving the existing logic or adjusting it.

I should just make this consistent with _warnings

I've added in deduplication step. However, I need to do more testing because im not actually sure if this codepath is ever hit.

I've noticed another issue with how failures are reported when a model throws an exception but i'll raise a new PR for that

themisvaltinos · 2025-07-09T21:38:04Z

sqlmesh/core/console.py

-        self._print(f"```\n\\[WARNING] {short_message}```\n\n")
+    @property
+    def captured_errors(self) -> str:
+        return self._render_alert_block("ERROR", self._errors)


how does this look in this ERROR case? is it like the caution one here: https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#alerts

🤦‍♂️ I should actually read the documentation I linked.

Yes, this should be CAUTION and not ERROR because there is no ERROR

I've updated this

great and thanks for providing a example of how it looks

eakmanrq · 2025-07-09T21:51:25Z

The warning looks great! Some things to think about (sorry if this is addressed in PR I just quickly reviewed it)

There are some warning that will keep showing up and users will want to ignore. The example from the screenshot is good since they might have accepted they are using a non-prod state db. Do you think they would be annoyed seeing this at the top every time? Should it potentially be collapsable and start collapsed so they can opt-in to seeing the information? The plan information on the PR comment does something like this currently
Checks have a max length and I wonder what users would prefer between truncating the normal check information and truncating warning/errors. Have you thought about that?
Are you aware of any warning that could get spammy that we send out in SQLMesh? I haven't worked on console since it added this warning logic so I'm not familiar with it but checking if we are concerned about noisy warnings spamming the top of the check

erindru · 2025-07-09T22:36:16Z

There are some warning that will keep showing up and users will want to ignore. Do you think they would be annoyed seeing this at the top every time?
Are you aware of any warning that could get spammy that we send out in SQLMesh?

Yeah, I did think about this. The thing is, using the warehouse for state is a completely sub-par experience so I don't think its the worst thing in the world if users get annoyed enough to use a proper database. If they're trying out SQLMesh in a POC capacity, it's also a good reminder that they are running in a sub-optimal mode.

The other warnings that might get spammy are:

the warning that gets triggered when we automatically adjust DuckDB config to prevent deadlocks if someone is using DuckDB for state. This probably wont be an issue in CICD
SQLGlot warnings like <sql> could not be semantically understood as it contains unsupported syntax which might not be actionable. Users should be seeing these locally as well though

Unfortunately, it looks like collapsed sections dont work with alert blocks:

The goal is to align with the CLI, so if the warnings are showing in the CLI then I think we should be surfacing them in the check output too.

Checks have a max length and I wonder what users would prefer between truncating the normal check information and truncating warning/errors. Have you thought about that?

I did not. Putting a limit on how much warning info is emitted (with a note to check the console output if it gets exceeded) sounds like a good idea, i'll implement it

erindru · 2025-07-09T22:45:36Z

Oh, interesting, it looks like collapsible sections can go inside alert blocks:

Warning

The PR plan produced the following warnings:

foo

Would that be preferable, coupled with truncating the warning text if it goes over a certain threshold?

erindru · 2025-07-10T01:45:08Z

sqlmesh/core/console.py

+        self.alert_block_max_content_length = int(kwargs.pop("alert_block_max_content_length", 500))
+        self.alert_block_collapsible_threshold = int(
+            kwargs.pop("alert_block_collapsible_threshold", 200)
+        )


I've hardcoded these thresholds for now but it would be a low lift to make them configurable if required.

alert_block_max_content_length - maximum amount of text to put inside a [!WARNING] block
alert_block_collapsible_threshold - if the text is longer than this, it isn't truncated (unless it exceeds alert_block_max_content_length), but it's wrapped inside a collapsible section within the [!WARNING] block

yes I feel the collapsible section within the block is much better and since it is collapsible I don't see the need to truncate and have the please check the console message anymore

themisvaltinos approved these changes Jul 9, 2025

View reviewed changes

erindru force-pushed the erin/cicd-bot-warning-error branch from bc3f02a to b94bc9c Compare July 10, 2025 01:41

erindru changed the title ~~Chore(cicd_bot): Show warnings in the check output~~ Chore(cicd_bot): Show logged warnings/errors in the check output Jul 10, 2025

erindru commented Jul 10, 2025

View reviewed changes

erindru force-pushed the erin/cicd-bot-warning-error branch from b94bc9c to 2622d0b Compare July 10, 2025 01:58

Chore(cicd_bot): Show warnings/errors in the check output

0cf8f9d

erindru force-pushed the erin/cicd-bot-warning-error branch from 2622d0b to 0cf8f9d Compare July 10, 2025 02:44

erindru merged commit 8be77dc into main Jul 10, 2025
27 checks passed

erindru deleted the erin/cicd-bot-warning-error branch July 10, 2025 20:48

Conversation

erindru commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

themisvaltinos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erindru Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eakmanrq commented Jul 9, 2025

Uh oh!

erindru commented Jul 9, 2025

Uh oh!

erindru commented Jul 9, 2025

Uh oh!

erindru Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

erindru commented Jul 9, 2025 •

edited

Loading

erindru Jul 9, 2025 •

edited

Loading

erindru Jul 10, 2025 •

edited

Loading