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

Update the webex notification provider and markdown #352

Merged
merged 2 commits into from
Mar 29, 2022

Conversation

ahothan
Copy link
Contributor

@ahothan ahothan commented Mar 23, 2022

Signed-off-by: ahothan ahothan@cisco.com

I have tested this against the latest webex API portal and verified I could get notifications in the targeted webex space.
Requires 2 additional parameters to make it work: a room ID and a Webex bot access token (both can be retrieved using the
webex developer web site after registering).
I have also enhanced the formatted markdown.
Example of webex space output:

Screen Shot 2022-03-23 at 11 32 38 AM

Signed-off-by: ahothan <ahothan@cisco.com>
@kingdonb
Copy link
Member

Thank you for this, if you are looking to add an example webex notification provider, the file where provider-specific examples are drawn from is: spec/v1beta1/provider.md

The example would land in the fluxcd docs under here:
https://fluxcd.io/docs/components/notification/provider/#example

(The current supplementary docs from each controller repo are pulled into the fluxcd/website repo on each deploy)

internal/notifier/webex.go Outdated Show resolved Hide resolved
@ahothan ahothan force-pushed the main branch 2 times, most recently from 17ae4d8 to eda9199 Compare March 24, 2022 17:11
@ahothan ahothan requested a review from stefanprodan March 25, 2022 13:49
@ahothan
Copy link
Contributor Author

ahothan commented Mar 25, 2022

documentation done

@ahothan
Copy link
Contributor Author

ahothan commented Mar 25, 2022

checking if this markdown format would be better:

<Alert summary in bold>
source: <Kind>/<namespace>/<name>
<message>
| <key>=<value> other kv pairs in manifests (all but summary)

@stefanprodan
Copy link
Member

@ahothan can you please post a print screen of how it looks now?

@ahothan
Copy link
Contributor Author

ahothan commented Mar 28, 2022

@stefanprodan
I wanted the summary to be first row as it would show where the notification comes from.
Depending on whether the alerts have a summary or not, the notification will use one or another layout.
The comment for the CreateMarkdown function describes how the markdown looks like based on summary present or not,
Here is a snapshot of a mock up (not real notification so dont pay attention to the exact severity)

In this snapshot, the Alert summary was set to "cc-k8s cluster notification" for the first 2,
thelast 2 is without a summary field.

Screen Shot 2022-03-28 at 7 37 54 AM

Please provide any suggestion...

@ahothan
Copy link
Contributor Author

ahothan commented Mar 28, 2022

not sure what to make of this error in the "kind" job:

Run kubectl -n notification-system get providers -oyaml
error: the server doesn't have a resource type "providers"
Error: Process completed with exit code 1.

@stefanprodan
Copy link
Member

The e2e tests are filing because your code is not properly formatted, see the first failing step in CI. To fix this, run make fmt then force push the changes.

@stefanprodan
Copy link
Member

I wanted the summary to be first row as it would show where the notification comes from.

This would make Webex different from all the others and I'm not for diverging from all the other providers.

Signed-off-by: ahothan <ahothan@cisco.com>
@ahothan
Copy link
Contributor Author

ahothan commented Mar 28, 2022

@stefanprodan
Reverted the markdown layout to be similar to others, ran make fmt

@ahothan
Copy link
Contributor Author

ahothan commented Mar 28, 2022

Snapshot from new flux bootstrap with latest version:

Screen Shot 2022-03-28 at 9 41 36 AM

@stefanprodan stefanprodan added enhancement New feature or request area/alerting Alerting related issues and PRs labels Mar 29, 2022
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @ahothan 🥇

@stefanprodan stefanprodan merged commit 6bc08e1 into fluxcd:main Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alerting Alerting related issues and PRs enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants