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

feat: Adding new datadog monitors chart #347

Merged
merged 11 commits into from
Nov 6, 2024
Merged

feat: Adding new datadog monitors chart #347

merged 11 commits into from
Nov 6, 2024

Conversation

scohen-nd
Copy link
Contributor

Adding new datadog monitors chart to allow users to use it as a subchart and manage datadog alerts via the datadog CRD's.

@scohen-nd scohen-nd requested a review from a team as a code owner November 5, 2024 20:02
Comment on lines +48 to +49
# -- (`map[string]interface{}`) Optional: monitor options
options:
Copy link
Member

Choose a reason for hiding this comment

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

I would ignore all the sub-keys here and just point to the upstream documentation instead as we are not likely to keep this list up to date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an option I thought about for the whole thing, in theory the names and the structure is exactly the same as the CRD - I could in theory point out to the CRD(though some of the info was not properly documented and I had to extract it from the code).

Comment on lines +8 to +9
# -- (`map[string]interface{}`) Required: monitor resource name, Required unique monitor resource name(needed to allow value overrides and used a datadog monitor resource name)
resourceName:
Copy link
Member

Choose a reason for hiding this comment

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

I would not create a real entry here and instead just have a large comment block. (Although I see you have disabled the entry below so I dont have strong opinions on this.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my original intention, but couldn't figure out how to properly format it so that helm-docs recognize the fields....

Copy link
Member

@LaikaN57 LaikaN57 left a comment

Choose a reason for hiding this comment

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

Left some non-blocking comments.

@scohen-nd scohen-nd merged commit 4228dd4 into main Nov 6, 2024
2 checks passed
@scohen-nd scohen-nd deleted the datadog_alerts branch November 6, 2024 17:07
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