Skip to content

Conversation

@RedOctober117
Copy link
Contributor

@RedOctober117 RedOctober117 commented Dec 16, 2024

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
Member

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
Contributor Author

@sorccu No worries, I look forward to it!

@sorccu
Copy link
Member

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.

Copy link
Member

@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
Contributor 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
Contributor Author

RedOctober117 commented Jan 5, 2025

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
@tnolet
Copy link
Member

tnolet commented Feb 24, 2025

@RedOctober117 @sorccu I was working on another alert channel just now (see #1026). I'm happy to pull this branch and update this PR based on the remarks @sorccu gave above. Most of the heavy lifting is already done, so this would be a nice addition for the next release.

@tnolet
Copy link
Member

tnolet commented Feb 26, 2025

This PR is surpassed by #1028
Thanks @RedOctober117 for the initial work!

@tnolet tnolet closed this Feb 26, 2025
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.

feat: add construct for MS Teams and Telegram alert channels in the CLI

3 participants