Skip to content

[ELE-517] Order slack messages by test run time ASC#989

Merged
ellakz merged 2 commits into
masterfrom
ele-517-order-slack-messages-by-test-run-time
Jul 13, 2023
Merged

[ELE-517] Order slack messages by test run time ASC#989
ellakz merged 2 commits into
masterfrom
ele-517-order-slack-messages-by-test-run-time

Conversation

@ellakz
Copy link
Copy Markdown
Contributor

@ellakz ellakz commented Jul 12, 2023

No description provided.

@linear
Copy link
Copy Markdown

linear Bot commented Jul 12, 2023

ELE-517 Order slack messages by test run time ASC

Is your feature request related to a problem? Please describe.
I'm confused when I scroll through the slack notifications because they are not clearly ordered.

Describe the solution you'd like
Order by test run timestamp ascending

Describe alternatives you've considered
Order by test name or model name. However, this would be inconsistent with the slack logic of having messages ordered by time.

Additional context

Would you be willing to contribute this feature?
Yes

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jul 12, 2023

👋 @ellakz
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.

@ellakz ellakz force-pushed the ele-517-order-slack-messages-by-test-run-time branch from 88bbc8c to 91b117e Compare July 12, 2023 15:46
@ellakz ellakz force-pushed the ele-517-order-slack-messages-by-test-run-time branch from 91b117e to 57b2f67 Compare July 12, 2023 15:49
@ellakz ellakz requested a review from IDoneShaveIt July 12, 2023 15:56
IDoneShaveIt
IDoneShaveIt approved these changes Jul 13, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@IDoneShaveIt IDoneShaveIt left a comment

Choose a reason for hiding this comment

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

Looking good with a small NIT

Comment thread elementary/monitor/data_monitoring/data_monitoring_alerts.py Outdated
grouped_alerts = by_table_group + by_alert_group
return sorted(
grouped_alerts,
key=lambda grp: min(alert.detected_at for alert in grp.alerts),
Copy link
Copy Markdown
Contributor

@IDoneShaveIt IDoneShaveIt Jul 13, 2023

Choose a reason for hiding this comment

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

I know this is a NIT but lets use explicit words grp -> group

@ellakz ellakz merged commit cb3e050 into master Jul 13, 2023
@ellakz ellakz deleted the ele-517-order-slack-messages-by-test-run-time branch July 13, 2023 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@IDoneShaveIt IDoneShaveIt IDoneShaveIt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ellakz @IDoneShaveIt