Skip to content

Fix for ELE-699, None default for --group-by#823

Merged
RoiTabach merged 1 commit intomasterfrom
ele-699-copied
Apr 24, 2023
Merged

Fix for ELE-699, None default for --group-by#823
RoiTabach merged 1 commit intomasterfrom
ele-699-copied

Conversation

@RoiTabach
Copy link
Copy Markdown
Contributor

Copied from CR 819 to make the automation work

Copied from CR 819 to make the automation work
@linear
Copy link
Copy Markdown

linear bot commented Apr 23, 2023

ELE-699 group_alerts_by in config.yml not respected

Describe the bug
The group_alerts_by value in config.yml is not respected because the option has a default value of alerts.

@click.option(
"--group-by",
type=click.Choice(["alert", "table"]),
default="alert",
help="Whether to group alerts by 'alert' or by 'table'",
)

So eventually when the Config is instantiated, the self.slack_group_alerts_by gets the default value that is first and not the config value.

self.slack_group_alerts_by = self._first_not_none(
slack_group_alerts_by,
slack_config.get("group_alerts_by"),
GroupingType.BY_ALERT.value,
)

To Reproduce
Steps to reproduce the behavior:

  1. Setup config.yml with group_alerts_by: "table"
  2. Run edr monitor
  3. Check in Slack that the alerts are not grouped by table

Expected behavior
My expectations were:

  • for the command to run using the config value of group_alerts_by
  • for the Slack alerts to be grouped by table

Environment (please complete the following information):

  • edr Version: 0.7.7
  • dbt package Version: 0.7.5

Additional context
My proposal is to make the default value in click.option to be None, since later the alert is the default fallback.

I can create a PR if you'd like.

@github-actions
Copy link
Copy Markdown
Contributor

👋 @RoiTabach
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

@RoiTabach RoiTabach merged commit 03192ab into master Apr 24, 2023
@RoiTabach RoiTabach deleted the ele-699-copied branch April 24, 2023 06: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.

1 participant