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.

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

Feature: Add silence datasource #403

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

dannyswebb
Copy link

SUMMARY

Grafana has the ability to support multiple alertmanager datasources. Add the ability for grafana_silence to use user defined datasources (or default to grafana if none supplied).

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

grafana_silence

ADDITIONAL INFORMATION

n/a

@dannyswebb dannyswebb requested review from rrey and seuf as code owners October 4, 2024 08:33
@Nemental
Copy link
Collaborator

Nemental commented Oct 4, 2024

Hey @dannyswebb thank you for your PR :)
Can you please add a changelog fragment and check the results of the failed workflows?

Copy link

codecov bot commented Oct 4, 2024

Codecov Report

Attention: Patch coverage is 42.85714% with 8 lines in your changes missing coverage. Please review.

Project coverage is 70.64%. Comparing base (53c9402) to head (4e0cbc1).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
plugins/modules/grafana_silence.py 42.85% 7 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #403       +/-   ##
===========================================
+ Coverage   45.46%   70.64%   +25.17%     
===========================================
  Files          21       11       -10     
  Lines        2228     1468      -760     
  Branches      437      326      -111     
===========================================
+ Hits         1013     1037       +24     
+ Misses       1174      257      -917     
- Partials       41      174      +133     
Flag Coverage Δ
integration 70.57% <42.85%> (?)
sanity ?
units ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dannyswebb dannyswebb force-pushed the feat/add_silence_datasource branch from b9fa5e5 to 539d502 Compare October 4, 2024 13:46
@Nemental
Copy link
Collaborator

Nemental commented Oct 8, 2024

Please use ruff and black for code linting/formatting.
https://github.com/ansible-collections/community.grafana?tab=readme-ov-file#contributing
Also there are some unit and sanity findings. Please take a look :) Thanks!

@dannyswebb dannyswebb force-pushed the feat/add_silence_datasource branch from 89f2e39 to 1dcea66 Compare October 9, 2024 09:02
@dannyswebb
Copy link
Author

Hey @dannyswebb thank you for your PR :) Can you please add a changelog fragment and check the results of the failed workflows?

fragment added, working through the failed workflows now. Sorry, been a couple years since I've touched mock tests and I'm a bit rusty.

@dannyswebb
Copy link
Author

Please use ruff and black for code linting/formatting. https://github.com/ansible-collections/community.grafana?tab=readme-ov-file#contributing Also there are some unit and sanity findings. Please take a look :) Thanks!

ruff and black linting done, working through the unit findings now.

Copy link
Collaborator

@Nemental Nemental left a comment

Choose a reason for hiding this comment

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

I've added a few suggestions. If grafana is defined as the default alertmanager in the documentation area, this must also be set in the code. I also made a few renaming changes. My code suggestions are not tested.

@@ -69,6 +69,12 @@
type: list
elements: dict
required: true
alertmanager_datasource:
description:
- Which alertmanager datasource to target
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Which alertmanager datasource to target
- Which alertmanager datasource to target.

@@ -352,6 +387,7 @@ def setup_module_object():
org_name=dict(type="str"),
skip_version_check=dict(type="bool", default=False),
starts_at=dict(type="str", required=True),
alertmanager_datasource=dict(type=str),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
alertmanager_datasource=dict(type=str),
alertmanager_datasource=dict(type=str, default="grafana"),

Comment on lines +252 to +255
if module.params.get("alertmanager_datasource", None):
self.alertmanager_path = self.datasource_by_name(
module.params["alertmanager_datasource"]
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if module.params.get("alertmanager_datasource", None):
self.alertmanager_path = self.datasource_by_name(
module.params["alertmanager_datasource"]
)
if module.params.get("alertmanager_datasource") != "grafana":
self.alertmanager_id = self.datasource_by_name(
module.params["alertmanager_datasource"]
)

Comment on lines +221 to +222
# Default here because you can't look up "grafana" DS via API
self.alertmanager_path = "grafana"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Default here because you can't look up "grafana" DS via API
self.alertmanager_path = "grafana"
self.alertmanager_id = module.params["alertmanager_datasource"]

@@ -280,7 +319,7 @@ def get_version(self):
raise GrafanaError("Failed to retrieve version from '%s'" % url)

def create_silence(self, comment, created_by, starts_at, ends_at, matchers):
url = "/api/alertmanager/grafana/api/v2/silences"
url = f"/api/alertmanager/{self.alertmanager_path}/api/v2/silences"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
url = f"/api/alertmanager/{self.alertmanager_path}/api/v2/silences"
url = f"/api/alertmanager/{self.alertmanager_id}/api/v2/silences"

@@ -297,7 +336,7 @@ def create_silence(self, comment, created_by, starts_at, ends_at, matchers):
return response

def get_silence(self, comment, created_by, starts_at, ends_at, matchers):
url = "/api/alertmanager/grafana/api/v2/silences"
url = f"/api/alertmanager/{self.alertmanager_path}/api/v2/silences"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
url = f"/api/alertmanager/{self.alertmanager_path}/api/v2/silences"
url = f"/api/alertmanager/{self.alertmanager_id}/api/v2/silences"

url = "/api/alertmanager/grafana/api/v2/silence/{SilenceId}".format(
SilenceId=silence_id
)
url = f"/api/alertmanager/{self.alertmanager_path}/api/v2/silence/{silence_id}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
url = f"/api/alertmanager/{self.alertmanager_path}/api/v2/silence/{silence_id}"
url = f"/api/alertmanager/{self.alertmanager_id}/api/v2/silence/{silence_id}"

response = self._send_request(url, headers=self.headers, method="GET")
return response

def get_silences(self):
url = "/api/alertmanager/grafana/api/v2/silences"
url = f"/api/alertmanager/{self.alertmanager_path}/api/v2/silences"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
url = f"/api/alertmanager/{self.alertmanager_path}/api/v2/silences"
url = f"/api/alertmanager/{self.alertmanager_id}/api/v2/silences"

url = "/api/alertmanager/grafana/api/v2/silence/{SilenceId}".format(
SilenceId=silence_id
)
url = f"/api/alertmanager/{self.alertmanager_path}/api/v2/silence/{silence_id}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
url = f"/api/alertmanager/{self.alertmanager_path}/api/v2/silence/{silence_id}"
url = f"/api/alertmanager/{self.alertmanager_id}/api/v2/silence/{silence_id}"

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.

3 participants