Skip to content

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

Closed
dimoschi wants to merge 1 commit intoelementary-data:masterfrom
dimoschi:fix-ele-699
Closed

Fix for ELE-699, None default for --group-by#819
dimoschi wants to merge 1 commit intoelementary-data:masterfrom
dimoschi:fix-ele-699

Conversation

@dimoschi
Copy link
Copy Markdown

To address issue #816 I've set the default value of --group-by option to None, since later on when the Config is instantiated, the self.slack_group_alerts_by gets alert as the last non-null value.

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

@dimoschi dimoschi changed the title [ELE-699] Fix for ELE-699, None default for --group-by Apr 23, 2023
@RoiTabach
Copy link
Copy Markdown
Contributor

@dimoschi does it now work for you in the config that caused the original issue?
I ran the automation on it anyway.

@dimoschi
Copy link
Copy Markdown
Author

@dimoschi does it now work for you in the config that caused the original issue? I ran the automation on it anyway.

Yes, it works perfectly fine with the config. FYI the config is the following:

slack:
  token: "definitely-not-a-token"
  channel_name: "data-ops"
  group_alerts_by: "table"
  alerts_config:
        alert_fields: ["table", "column", "result_message", "test_parameters", "test_query", "test_results_sample", "description","subscribers"]

and the slack post was the following:
image

@dimoschi
Copy link
Copy Markdown
Author

Tests failed though 😞

@RoiTabach
Copy link
Copy Markdown
Contributor

Copied the changes to this PR
#823
In order for the automation to be able to run.

@RoiTabach RoiTabach closed this Apr 23, 2023
@dimoschi dimoschi deleted the fix-ele-699 branch April 23, 2023 16:34
@dimoschi
Copy link
Copy Markdown
Author

dimoschi commented Apr 23, 2023

@RoiTabach I guess it was because the Pr comes from a forked repo? FYI, I wasn't able to push a new branch, that's why I forked it.

Also, if you can help me setup testing suite, I'm willing to write tests.

@RoiTabach
Copy link
Copy Markdown
Contributor

@dimoschi it's merged

@RoiTabach
Copy link
Copy Markdown
Contributor

(@dimoschi Sending you a Slack message in the elementary community slack space)

@dimoschi dimoschi restored the fix-ele-699 branch April 24, 2023 07:10
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.

2 participants