diff --git a/docs/integrations/github.md b/docs/integrations/github.md index b0437242c9..5b950f480b 100644 --- a/docs/integrations/github.md +++ b/docs/integrations/github.md @@ -5,6 +5,7 @@ The GitHub Actions CI/CD Bot enables teams to automate their SQLMesh projects using GitHub Actions. It can be configured to perform the following things: * Automatically run unit tests on PRs +* Automatically run the linter on PRs * Automatically create PR environments that represent the code changes in the PR * Automatically categorize and backfill data for models that have changed * Automatically deploy changes to production with automatic data gap prevention and merge the PR @@ -350,6 +351,7 @@ These can be used to potentially trigger follow up steps in the workflow. These are the possible outputs (based on how the bot is configured) that are created by the bot: * `run_unit_tests` +* `linter` * `has_required_approval` * `pr_environment_synced` * `prod_plan_preview` @@ -373,6 +375,8 @@ In addition, there are custom outputs listed below: * `created_pr_environment` - set to `"true"` (a string with a value of `true`) if a PR environment was created for the first time. It is absent, or considered empty string if you check for it, if it is not created for the first time * `pr_environment_name` - the name of the PR environment. It is output whenever PR environment synced check reaches a conclusion. Therefore make sure to check the status of `created_pr_environment` or `pr_environment_synced` before acting on this output +Note: The `linter` step will run only if it's enabled in the project's configuration (`config.yaml` / `config.py`). The step will fail if the linter finds errors, otherwise it'll output only the warnings. + ## Custom Workflow Configuration You can configure each individual action to run as a separate step. This can allow for more complex workflows or integrating specific steps with other actions you want to trigger. Run `sqlmesh_cicd github` to see a list of commands that can be supplied and their potential options. ```bash @@ -460,6 +464,10 @@ jobs: ## Example Screenshots ### Automated Unit Tests with Error Summary ![Automated Unit Tests with Error Summary](github/github_test_summary.png) +### Automated Linting with Error Summary +![Automated Linting with Error Summary](github/linter_errors.png) +### Automated Linting with Warning Summary +![Automated Linting with Warning Summary](github/linter_warnings.png) ### Automatically create PR Environments that represent the code changes in the PR ![Environment Summary](github/github_env_summary.png) ### Enforce that certain reviewers have approved of the PR before it can be merged diff --git a/docs/integrations/github/linter_errors.png b/docs/integrations/github/linter_errors.png new file mode 100644 index 0000000000..d12b9f0ab2 Binary files /dev/null and b/docs/integrations/github/linter_errors.png differ diff --git a/docs/integrations/github/linter_warnings.png b/docs/integrations/github/linter_warnings.png new file mode 100644 index 0000000000..609ea1a524 Binary files /dev/null and b/docs/integrations/github/linter_warnings.png differ diff --git a/sqlmesh/core/console.py b/sqlmesh/core/console.py index c1dd42d78e..369352214f 100644 --- a/sqlmesh/core/console.py +++ b/sqlmesh/core/console.py @@ -2164,7 +2164,6 @@ def _show_categorized_snapshots(self, plan: Plan, default_catalog: t.Optional[st def log_test_results( self, result: unittest.result.TestResult, output: t.Optional[str], target_dialect: str ) -> None: - # import ipywidgets as widgets if result.wasSuccessful(): self._print( f"**Successfully Ran `{str(result.testsRun)}` Tests Against `{target_dialect}`**\n\n" diff --git a/sqlmesh/core/context.py b/sqlmesh/core/context.py index e5896df8db..004e5c1e28 100644 --- a/sqlmesh/core/context.py +++ b/sqlmesh/core/context.py @@ -2394,7 +2394,10 @@ def _get_models_for_interval_end( ) return models_for_interval_end - def lint_models(self, models: t.Optional[t.Iterable[t.Union[str, Model]]] = None) -> None: + def lint_models( + self, + models: t.Optional[t.Iterable[t.Union[str, Model]]] = None, + ) -> None: found_error = False model_list = ( @@ -2403,7 +2406,7 @@ def lint_models(self, models: t.Optional[t.Iterable[t.Union[str, Model]]] = None for model in model_list: # Linter may be `None` if the context is not loaded yet if linter := self._linters.get(model.project): - found_error = linter.lint_model(model) or found_error + found_error = linter.lint_model(model, console=self.console) or found_error if found_error: raise LinterError( diff --git a/sqlmesh/core/linter/definition.py b/sqlmesh/core/linter/definition.py index 27d33fd5e1..5628a43f5c 100644 --- a/sqlmesh/core/linter/definition.py +++ b/sqlmesh/core/linter/definition.py @@ -7,7 +7,7 @@ from sqlmesh.core.model import Model from sqlmesh.utils.errors import raise_config_error -from sqlmesh.core.console import get_console +from sqlmesh.core.console import get_console, Console from sqlmesh.core.linter.rule import RuleSet @@ -50,7 +50,7 @@ def from_rules(cls, all_rules: RuleSet, config: LinterConfig) -> Linter: return Linter(config.enabled, all_rules, rules, warn_rules) - def lint_model(self, model: Model) -> bool: + def lint_model(self, model: Model, console: Console = get_console()) -> bool: if not self.enabled: return False @@ -63,10 +63,10 @@ def lint_model(self, model: Model) -> bool: warn_violations = warn_rules.check_model(model) if warn_violations: - get_console().show_linter_violations(warn_violations, model) + console.show_linter_violations(warn_violations, model) if error_violations: - get_console().show_linter_violations(error_violations, model, is_error=True) + console.show_linter_violations(error_violations, model, is_error=True) return True return False diff --git a/sqlmesh/integrations/github/cicd/command.py b/sqlmesh/integrations/github/cicd/command.py index 3aa46c5e95..b66e32fb6c 100644 --- a/sqlmesh/integrations/github/cicd/command.py +++ b/sqlmesh/integrations/github/cicd/command.py @@ -13,7 +13,7 @@ GithubController, TestFailure, ) -from sqlmesh.utils.errors import CICDBotError, ConflictingPlanError, PlanError +from sqlmesh.utils.errors import CICDBotError, ConflictingPlanError, PlanError, LinterError logger = logging.getLogger(__name__) @@ -80,6 +80,25 @@ def _run_tests(controller: GithubController) -> bool: return False +def _run_linter(controller: GithubController) -> bool: + controller.update_linter_check(status=GithubCheckStatus.IN_PROGRESS) + try: + controller.run_linter() + except LinterError: + controller.update_linter_check( + status=GithubCheckStatus.COMPLETED, + conclusion=GithubCheckConclusion.FAILURE, + ) + return False + + controller.update_linter_check( + status=GithubCheckStatus.COMPLETED, + conclusion=GithubCheckConclusion.SUCCESS, + ) + + return True + + @github.command() @click.pass_context @cli_analytics @@ -204,11 +223,13 @@ def _run_all(controller: GithubController) -> None: has_required_approval = True else: raise CICDBotError(f"Unsupported command: {command}") + controller.update_linter_check(status=GithubCheckStatus.QUEUED) controller.update_pr_environment_check(status=GithubCheckStatus.QUEUED) controller.update_prod_plan_preview_check(status=GithubCheckStatus.QUEUED) controller.update_test_check(status=GithubCheckStatus.QUEUED) if is_auto_deploying_prod: controller.update_prod_environment_check(status=GithubCheckStatus.QUEUED) + linter_passed = _run_linter(controller) tests_passed = _run_tests(controller) if controller.do_required_approval_check: if has_required_approval: @@ -218,23 +239,25 @@ def _run_all(controller: GithubController) -> None: else: controller.update_required_approval_check(status=GithubCheckStatus.QUEUED) has_required_approval = _check_required_approvers(controller) - if not tests_passed: + if not tests_passed or not linter_passed: controller.update_pr_environment_check( status=GithubCheckStatus.COMPLETED, - exception=TestFailure(), + exception=LinterError("") if not linter_passed else TestFailure(), ) controller.update_prod_plan_preview_check( status=GithubCheckStatus.COMPLETED, conclusion=GithubCheckConclusion.SKIPPED, - summary="Unit Test(s) Failed so skipping creating prod plan", + summary="Linter or Unit Test(s) failed so skipping creating prod plan", ) if is_auto_deploying_prod: controller.update_prod_environment_check( status=GithubCheckStatus.COMPLETED, conclusion=GithubCheckConclusion.SKIPPED, - skip_reason="Unit Test(s) Failed so skipping deploying to production", + skip_reason="Linter or Unit Test(s) failed so skipping deploying to production", ) - raise CICDBotError("Failed to run tests. See check status for more information.") + + raise CICDBotError("Linter or Unit Test(s) failed. See check status for more information.") + pr_environment_updated = _update_pr_environment(controller) prod_plan_generated = False if pr_environment_updated: diff --git a/sqlmesh/integrations/github/cicd/controller.py b/sqlmesh/integrations/github/cicd/controller.py index b4364d6439..a8da39a81d 100644 --- a/sqlmesh/integrations/github/cicd/controller.py +++ b/sqlmesh/integrations/github/cicd/controller.py @@ -29,6 +29,7 @@ format_intervals, ) from sqlmesh.core.user import User +from sqlmesh.core.config import Config from sqlmesh.integrations.github.cicd.config import GithubCICDBotConfig from sqlmesh.utils import word_characters_only, Verbosity from sqlmesh.utils.concurrency import NodeExecutionFailedError @@ -38,6 +39,7 @@ NoChangesPlanError, PlanError, UncategorizedPlanError, + LinterError, ) from sqlmesh.utils.pydantic import PydanticModel @@ -50,8 +52,6 @@ from github.PullRequestReview import PullRequestReview from github.Repository import Repository - from sqlmesh.core.config import Config - logger = logging.getLogger(__name__) @@ -326,10 +326,7 @@ def __init__( if review.state.lower() == "approved" } logger.debug(f"Approvers: {', '.join(self._approvers)}") - self._context: Context = Context( - paths=self._paths, - config=self.config, - ) + self._context: Context = Context(paths=self._paths, config=self.config) @property def deploy_command_enabled(self) -> bool: @@ -394,6 +391,7 @@ def pr_plan(self) -> Plan: self._pr_plan_builder = self._context.plan_builder( environment=self.pr_environment_name, skip_tests=True, + skip_linter=True, categorizer_config=self.bot_config.auto_categorize_changes, start=self.bot_config.default_pr_start, skip_backfill=self.bot_config.skip_pr_backfill, @@ -409,6 +407,7 @@ def prod_plan(self) -> Plan: c.PROD, no_gaps=True, skip_tests=True, + skip_linter=True, categorizer_config=self.bot_config.auto_categorize_changes, run=self.bot_config.run_on_deploy_to_prod, ) @@ -423,6 +422,7 @@ def prod_plan_with_gaps(self) -> Plan: no_gaps=False, no_auto_categorization=True, skip_tests=True, + skip_linter=True, run=self.bot_config.run_on_deploy_to_prod, ) assert self._prod_plan_with_gaps_builder @@ -478,6 +478,13 @@ def run_tests(self) -> t.Tuple[unittest.result.TestResult, str]: """ return self._context._run_tests(verbosity=Verbosity.VERBOSE) + def run_linter(self) -> None: + """ + Run linter for the PR + """ + self._console.clear_captured_outputs() + self._context.lint_models() + def _get_or_create_comment(self, header: str = BOT_HEADER_MSG) -> IssueComment: comment = seq_get( [comment for comment in self._issue.get_comments() if header in comment.body], @@ -654,6 +661,37 @@ def _update_check_handler( full_summary=summary, ) + def update_linter_check( + self, + status: GithubCheckStatus, + conclusion: t.Optional[GithubCheckConclusion] = None, + ) -> None: + if not self._context.config.linter.enabled: + return + + def conclusion_handler( + conclusion: GithubCheckConclusion, + ) -> t.Tuple[GithubCheckConclusion, str, t.Optional[str]]: + linter_summary = self._console.consume_captured_output() or "Linter Success" + + title = "Linter results" + + return conclusion, title, linter_summary + + self._update_check_handler( + check_name="SQLMesh - Linter", + status=status, + conclusion=conclusion, + status_handler=lambda status: ( + { + GithubCheckStatus.IN_PROGRESS: "Running linter", + GithubCheckStatus.QUEUED: "Waiting to Run linter", + }[status], + None, + ), + conclusion_handler=conclusion_handler, + ) + def update_test_check( self, status: GithubCheckStatus, @@ -751,7 +789,7 @@ def update_pr_environment_check( Updates the status of the merge commit for the PR environment. """ conclusion: t.Optional[GithubCheckConclusion] = None - if isinstance(exception, (NoChangesPlanError, TestFailure)): + if isinstance(exception, (NoChangesPlanError, TestFailure, LinterError)): conclusion = GithubCheckConclusion.SKIPPED elif isinstance(exception, UncategorizedPlanError): conclusion = GithubCheckConclusion.ACTION_REQUIRED diff --git a/tests/integrations/github/cicd/fixtures.py b/tests/integrations/github/cicd/fixtures.py index 1e3ac6664a..2a60378c78 100644 --- a/tests/integrations/github/cicd/fixtures.py +++ b/tests/integrations/github/cicd/fixtures.py @@ -3,6 +3,7 @@ import pytest from pytest_mock.plugin import MockerFixture +from sqlmesh.core.config import Config from sqlmesh.core.console import set_console, get_console, MarkdownConsole from sqlmesh.integrations.github.cicd.config import GithubCICDBotConfig from sqlmesh.integrations.github.cicd.controller import ( @@ -67,6 +68,7 @@ def _make_function( merge_state_status: MergeStateStatus = MergeStateStatus.CLEAN, bot_config: t.Optional[GithubCICDBotConfig] = None, mock_out_context: bool = True, + config: t.Optional[t.Union[Config, str]] = None, ) -> GithubController: if mock_out_context: mocker.patch("sqlmesh.core.context.Context.apply", mocker.MagicMock()) @@ -97,6 +99,7 @@ def _make_function( else GithubEvent.from_obj(event_path) ), client=client, + config=config, ) finally: set_console(orig_console) diff --git a/tests/integrations/github/cicd/test_github_commands.py b/tests/integrations/github/cicd/test_github_commands.py index f50a76fd6f..8c305bc20e 100644 --- a/tests/integrations/github/cicd/test_github_commands.py +++ b/tests/integrations/github/cicd/test_github_commands.py @@ -497,7 +497,7 @@ def test_run_all_test_failed( ) assert ( prod_plan_preview_checks_runs[1]["output"]["summary"] - == "Unit Test(s) Failed so skipping creating prod plan" + == "Linter or Unit Test(s) failed so skipping creating prod plan" ) assert "SQLMesh - PR Environment Synced" in controller._check_run_mapping @@ -625,7 +625,7 @@ def test_run_all_test_exception( ) assert ( prod_plan_preview_checks_runs[1]["output"]["summary"] - == "Unit Test(s) Failed so skipping creating prod plan" + == "Linter or Unit Test(s) failed so skipping creating prod plan" ) assert "SQLMesh - PR Environment Synced" in controller._check_run_mapping diff --git a/tests/integrations/github/cicd/test_integration.py b/tests/integrations/github/cicd/test_integration.py index 35afbb89a3..1fe37c6bee 100644 --- a/tests/integrations/github/cicd/test_integration.py +++ b/tests/integrations/github/cicd/test_integration.py @@ -13,7 +13,7 @@ from pytest_mock.plugin import MockerFixture from sqlglot import exp -from sqlmesh.core.config import CategorizerConfig +from sqlmesh.core.config import CategorizerConfig, Config, ModelDefaultsConfig, LinterConfig from sqlmesh.core.engine_adapter.shared import DataObject from sqlmesh.core.user import User, UserRole from sqlmesh.integrations.github.cicd import command @@ -52,6 +52,138 @@ def get_columns( return controller._context.engine_adapter.columns(table) +@time_machine.travel("2023-01-01 15:00:00 UTC") +def test_linter( + github_client, + make_controller, + make_mock_check_run, + make_mock_issue_comment, + make_pull_request_review, + tmp_path: pathlib.Path, + mocker: MockerFixture, +): + """ + PR with the Linter enabled will contain a new check with the linter specific output. + + Scenarios: + - PR with linter errors leads to job failures & skips + - PR with linter warnings leads to job successes + """ + mock_repo = github_client.get_repo() + mock_repo.create_check_run = mocker.MagicMock( + side_effect=lambda **kwargs: make_mock_check_run(**kwargs) + ) + + created_comments: t.List[MockIssueComment] = [] + mock_issue = mock_repo.get_issue() + mock_issue.create_comment = mocker.MagicMock( + side_effect=lambda comment: make_mock_issue_comment( + comment=comment, created_comments=created_comments + ) + ) + mock_issue.get_comments = mocker.MagicMock(side_effect=lambda: created_comments) + + mock_pull_request = mock_repo.get_pull() + mock_pull_request.get_reviews = mocker.MagicMock( + side_effect=lambda: [make_pull_request_review(username="test_github", state="APPROVED")] + ) + mock_pull_request.merged = False + mock_pull_request.merge = mocker.MagicMock() + + # Case 1: Test for linter errors + config = Config( + model_defaults=ModelDefaultsConfig(dialect="duckdb"), + linter=LinterConfig(enabled=True, rules="ALL"), + ) + + controller = make_controller( + "tests/fixtures/github/pull_request_synchronized.json", + github_client, + bot_config=GithubCICDBotConfig( + merge_method=MergeMethod.MERGE, + invalidate_environment_after_deploy=False, + auto_categorize_changes=CategorizerConfig.all_full(), + default_pr_start=None, + skip_pr_backfill=False, + ), + mock_out_context=False, + config=config, + ) + + github_output_file = tmp_path / "github_output.txt" + + with mock.patch.dict(os.environ, {"GITHUB_OUTPUT": str(github_output_file)}): + with pytest.raises(CICDBotError): + command._run_all(controller) + + assert "SQLMesh - Linter" in controller._check_run_mapping + linter_checks_runs = controller._check_run_mapping["SQLMesh - Linter"].all_kwargs + assert "Linter **errors** for" in linter_checks_runs[2]["output"]["summary"] + assert GithubCheckConclusion(linter_checks_runs[2]["conclusion"]).is_failure + + for check in ( + "SQLMesh - PR Environment Synced", + "SQLMesh - Prod Plan Preview", + ): + assert check in controller._check_run_mapping + check_runs = controller._check_run_mapping[check].all_kwargs + assert GithubCheckConclusion(check_runs[-1]["conclusion"]).is_skipped + + with open(github_output_file, "r", encoding="utf-8") as f: + output = f.read() + assert ( + output + == "linter=failure\nrun_unit_tests=success\npr_environment_name=hello_world_2\npr_environment_synced=skipped\nprod_plan_preview=skipped\n" + ) + + # empty github file for next case + open(github_output_file, "w").close() + + # Case 2: Test for linter warnings + config = Config( + model_defaults=ModelDefaultsConfig(dialect="duckdb"), + linter=LinterConfig(enabled=True, warn_rules="ALL"), + ) + + controller = make_controller( + "tests/fixtures/github/pull_request_synchronized.json", + github_client, + bot_config=GithubCICDBotConfig( + merge_method=MergeMethod.MERGE, + invalidate_environment_after_deploy=False, + auto_categorize_changes=CategorizerConfig.all_full(), + default_pr_start=None, + skip_pr_backfill=False, + ), + mock_out_context=False, + config=config, + ) + + with mock.patch.dict(os.environ, {"GITHUB_OUTPUT": str(github_output_file)}): + command._run_all(controller) + + assert "SQLMesh - Linter" in controller._check_run_mapping + linter_checks_runs = controller._check_run_mapping["SQLMesh - Linter"].all_kwargs + assert "Linter warnings for" in linter_checks_runs[-1]["output"]["summary"] + assert GithubCheckConclusion(linter_checks_runs[-1]["conclusion"]).is_success + + for check in ( + "SQLMesh - Run Unit Tests", + "SQLMesh - PR Environment Synced", + "SQLMesh - Prod Plan Preview", + ): + assert check in controller._check_run_mapping + check_runs = controller._check_run_mapping[check].all_kwargs + assert GithubCheckConclusion(check_runs[-1]["conclusion"]).is_success + + with open(github_output_file, "r", encoding="utf-8") as f: + output = f.read() + assert ( + output + == "linter=success\nrun_unit_tests=success\ncreated_pr_environment=true\npr_environment_name=hello_world_2\npr_environment_synced=success\nprod_plan_preview=success\n" + ) + + @time_machine.travel("2023-01-01 15:00:00 UTC") def test_merge_pr_has_non_breaking_change( github_client,