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

Feature: Add MSTeams and Telegram channels to CLI #1004

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

Feature: Add MSTeams and Telegram channels to CLI #1004

wants to merge 7 commits into from

Conversation

RedOctober117
Copy link

@RedOctober117 RedOctober117 commented Dec 16, 2024 •
edited
Loading

I hereby confirm that I followed the code guidelines found at engineering guidelines

^ (access is denied to me)

Affected Components

  • CLI
  • Create CLI
  • Test
  • Docs
  • Examples
  • Other

Notes for the Reviewer

Completely unfamiliar with the code base, so consider this my modest attempt at a contribution. It does not look like the Checkly API has native support for Teams/Telegram endpoints, so I went through the Webhook endpoint with templates identical to what the webpage offers.

Open to all feedback and will fix whatever is wrong.

Resolves #995

New Dependency Submission

N/A

@sorccu
Copy link
Collaborator

sorccu commented Dec 20, 2024

Hi @RedOctober117, thanks a lot for the contribution! I've been super busy lately but intend to review this soon.

@RedOctober117
Copy link
Author

@sorccu No worries, I look forward to it!

@sorccu
Copy link
Collaborator

sorccu commented Jan 4, 2025

Hey, it's been a while but I took a look at these and I really appreciate the contribution. There are a few things I would like you to address:

  1. Make sure your editor is using our eslint rules. Currently, both files have extra semicolons and some whitespace issues, including a different indentation style in the telegram alert channel file. Normally our lint CI would let you know about that but unfortunately our CI jobs do not currently work well with forks.
  2. Prefer camelCase over snake_case.
  3. There are some unused imports which should be removed.
  4. The template for the MS Teams alert channel is not what we currently use in the web UI. I am not sure if it has changed recently, but at the very least the link for the action button at the very bottom is wrong since it does not go to Checkly. Here is what we use:
{
  "type": "message",
  "attachments": [
    {
      "contentType": "application/vnd.microsoft.card.adaptive",
      "contentUrl": null,
      "content": {
        "$schema": "http://adaptivecards.io/schemas/adaptive-card.json",
        "type": "AdaptiveCard",
        "version": "1.2",
        "body": [
          {
            "type": "Container",
            "items": [
              {
                "type": "TextBlock",
                "text": "{{ALERT_TITLE}}",
                "weight": "bolder",
                "size": "medium"
              },
              {
                "type": "ColumnSet",
                "columns": [
                  {
                    "type": "Column",
                    "width": "stretch",
                    "items": [
                      {
                        "type": "TextBlock",
                        "text": "Response time: {{RESPONSE_TIME}}ms",
                        "wrap": true
                      },
                      {
                        "type": "TextBlock",
                        "text": "Location: {{RUN_LOCATION}}",
                        "wrap": true
                      },
                      {
                        "type": "TextBlock",
                        "text": "Timestamp: {{STARTED_AT}}",
                        "wrap": true
                      },
                      {{#if GROUP_NAME}}
                      {
                        "type": "TextBlock",
                        "text": "Group: {{GROUP_NAME}}",
                        "wrap": true
                      },
                      {{/if}}
                      {
                        "type": "TextBlock",
                        "text": "Tags: {{#each TAGS}} {{this}} {{#unless @last}},{{/unless}} {{/each}}",
                        "wrap": true
                      }
                    ]
                  }
                ]
              }
            ]
          }
        ],
        "actions": [
          {
            "type": "Action.OpenUrl",
            "title": "View in Checkly",
            "url": "{{RESULT_LINK}}"
          }
        ]
      }
    }
  ]
}
  1. You had no way of knowing this, but you should use the WEBHOOK_MSTEAMS as the webhookType in the config: {} object for the MS Teams hook, and WEBHOOK_TELEGRAM for the Telegram hook.
  2. For reasons such as the above, both the Telegram and the MS Teams alert channel should extend WebhookAlertChannel rather than the base AlertChannel. I think that would make it far easier to ensure the right things are getting sent.
  3. The MS Teams Alert Channel has a channel_name but it will not be used for anything and should be removed. The channel is chosen when creating a flow in MS Teams, as shown in our docs: https://www.checklyhq.com/docs/integrations/msteams/
  4. The Telegram alert channel seems to use the GET method but it should almost certainly be POST. Additionally, the payload is a little different than the one we use. As a result it probably doesn't work at all right now. Here is what we use (as a snippet of the actual JSON payload sent to the backend):
"template": "chat_id=-2353995&parse_mode=HTML&text=<b>{{ALERT_TITLE}}</b> at {{STARTED_AT}} in {{RUN_LOCATION}} ({{RESPONSE_TIME}}ms)\nTags: {{#each TAGS}} <i><b>{{this}}</b></i> {{#unless @last}},{{/unless}} {{/each}}\n<a href=\"{{RESULT_LINK}}\">View check result</a>\n"
  1. There would really have to be tests for these. Now to be fair the backend would unfortunately probably accept an invalid or unusable payload, but these alert types should be added to e2e tests anyway.

sorccu
sorccu requested changes Jan 4, 2025
View reviewed changes
Copy link
Collaborator

@sorccu sorccu left a comment

Choose a reason for hiding this comment

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

Please check my previous comment for details.

@RedOctober117
Copy link
Author

I don't know how to write tests in ts yet, but that commit should cover the rest. I did test the old templates though, and I can confirm they work.

@RedOctober117
Copy link
Author

RedOctober117 commented Jan 5, 2025 •
edited
Loading

I believe this is the right place for alert tests: packages\cli\e2e\__tests__\fixtures\test-project\src\alert-channels.ts. Please advise if not, and how I should edit them, if need be.

Thanks!

@RedOctober117 RedOctober117 requested a review from sorccu January 5, 2025 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorccu sorccu Awaiting requested review from sorccu

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.

feat: add construct for MS Teams and Telegram alert channels in the CLI
2 participants
@RedOctober117 @sorccu