Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

A more detailed list of clients #63

Merged
merged 7 commits into from
Nov 30, 2023
Merged

A more detailed list of clients #63

merged 7 commits into from
Nov 30, 2023

Conversation

IbraAoad
Copy link
Contributor

Issue

Rather than listing a simple list of strings in the config file, we should have a more detailed list of clients. #60

Solution

Adjust the server logic to receive both client UUID and match it with it's key and then show the client name in the dashboard instead of the id

Testing Instructions

  1. Add a client's details in the cos-alerter config that contain (valid UUID, Key and client's Name)
  2. Run COS Alerter.
  3. Configure Alertmanager to route the watchdog alert to cos-alerter using the same UUID&Key
  4. Validate that heartbeat is received in dashboard

Release Notes

User is now able to set uuid, key and client's name in cos-alerter config rather than just the client ID

github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 27, 2023
View reviewed changes
cos_alerter/server.py Fixed Show fixed Hide fixed
cos_alerter/server.py Fixed Show fixed Hide fixed
dstathis
dstathis reviewed Nov 27, 2023
View reviewed changes
Copy link
Contributor

@dstathis dstathis left a comment

Choose a reason for hiding this comment

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

Looks very good overall. Just a few comments. (Some of them might require a lot of code changes unfortunately)

cos_alerter/server.py Outdated Show resolved Hide resolved
cos_alerter/server.py Outdated Show resolved Hide resolved
cos_alerter/server.py Outdated Show resolved Hide resolved
cos_alerter/config-defaults.yaml Outdated Show resolved Hide resolved
sed-i
sed-i reviewed Nov 27, 2023
View reviewed changes
cos_alerter/alerter.py Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
cos_alerter/server.py Outdated Show resolved Hide resolved
tests/test_server.py Outdated Show resolved Hide resolved
@IbraAoad IbraAoad mentioned this pull request Nov 28, 2023
Open
dstathis
dstathis requested changes Nov 29, 2023
View reviewed changes
cos_alerter/alerter.py Outdated Show resolved Hide resolved
cos_alerter/alerter.py Outdated Show resolved Hide resolved
cos_alerter/config-defaults.yaml Outdated Show resolved Hide resolved
@dstathis
Copy link
Contributor

I almost forgot, you will also need a version bump and a changelog entry.

dstathis
dstathis approved these changes Nov 30, 2023
View reviewed changes
cos_alerter/alerter.py Show resolved Hide resolved
cos_alerter/config-defaults.yaml Outdated Show resolved Hide resolved
cos_alerter/config-defaults.yaml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@IbraAoad IbraAoad merged commit 63c09cf into main Nov 30, 2023
7 checks passed
@IbraAoad IbraAoad deleted the rich_client_config branch November 30, 2023 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sed-i sed-i sed-i left review comments

@dstathis dstathis dstathis approved these changes

@Abuelodelanada Abuelodelanada Awaiting requested review from Abuelodelanada

@lucabello lucabello Awaiting requested review from lucabello

@PietroPasotti PietroPasotti Awaiting requested review from PietroPasotti

@simskij simskij Awaiting requested review from simskij

@mmkay mmkay Awaiting requested review from mmkay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@IbraAoad @dstathis @sed-i