feat(slack): derive chat.update URL from configured api_url base#5225
feat(slack): derive chat.update URL from configured api_url base#5225santiagofn wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SoloJacobs
left a comment
There was a problem hiding this comment.
It looks like this was largely AI-generated. We'd ask that contributions be reviewed and understood by the author before submitting, including making sure the commit message accurately describes what the code actually does. The message here says "refactor" but the change alters behavior. Happy to look at it again once it's been properly reviewed on your end.
hi, @SoloJacobs , sorry for the delay. one comment before all: this is still a draft, that's why i didn't share it on slack. regarding the comments, i not only reviewed but wrote code, the PR was not solely generated by AI. i chose a "refactor" label because the current behaviour was not change (the same URLs are generated in the current flow). what the PR did add was the ability of creating URLs for other methods... but they're not in use nowadays. anyways, it might be worth using a "feature" if you believe that potential use warrants it. |
|
Thanks for the context. To be more precise though: The commit description says "move", which implies pure relocation.
So, the "refactor" part is not really the issue, you could leave it. Although, |
f912d94 to
0649d19
Compare
42ea34e to
bd70a71
Compare
Moves SlackConfig (and the related SlackAction, SlackConfirmationField, SlackField types plus DefaultSlackConfig) from config/notifiers.go into a new notify/slack/config.go, following the same pattern used recently for the mattermost, jira, and incidentio notifiers. While moving them, the Slack prefix is dropped per the package-naming guidance in https://go.dev/blog/package-names ("Avoid repetition"): SlackConfig -> Config, DefaultSlackConfig -> DefaultConfig, SlackAction -> Action, SlackConfirmationField -> ConfirmationField, SlackField -> Field. External callers now refer to slack.Config etc., reading better than the previous slack.SlackConfig stutter. The Slack-specific unmarshal tests are moved alongside them into notify/slack/config_test.go. config.Receiver now refers to *slack.Config, and notify/slack no longer needs to import the top-level config package. No behavioral changes. Signed-off-by: Santiago Fernández Núñez <santiago.nunez@nubank.com.br> Co-authored-by: Cursor <cursoragent@cursor.com>
Previously the Slack notifier hardcoded the chat.update endpoint to "https://slack.com/api/chat.update" regardless of the configured api_url. Deployments pointing api_url at a Slack-compatible proxy or forwarder were therefore forced back onto slack.com for update calls, which is the wrong host for proxy users. Move the URL handling into a new internal/apiurl package, and route chat.update (and any future Slack Web API method) through the same api_url / api_url_file base as the initial chat.postMessage call. Standard configurations are unaffected; the generated URL is byte identical. The Resolver also reads api_url_file on every call, matching previous behaviour so that file rotations apply to the next notification without restarting Alertmanager. Signed-off-by: Santiago Fernández Núñez <santiago.nunez@nubank.com.br> Co-authored-by: Cursor <cursoragent@cursor.com>
bd70a71 to
dd56508
Compare
Part 3 of a series implementing threaded message support for the Slack notifier. Full picture: #5150. Depends on #5239.
Summary
Moves the Slack notifier's URL resolution into a new internal package
notify/slack/internal/apiurl, and routes both the initialchat.postMessagerequest and thechat.updaterequest (used byupdate_message: true) through the same configuredapi_url/api_url_filebase.Behavior change
Previously the
chat.updateendpoint was hardcoded tohttps://slack.com/api/chat.updateregardless ofapi_url. With this PR, the same base used forchat.postMessageis reused forchat.update.api_urlunset or pointing athttps://slack.com/api/chat.postMessage) are unaffected: the generated URL is byte-identical to today.api_urlat a Slack-API-compatible proxy or forwarder (e.g.https://my-proxy.example.com/slack/api/chat.postMessage) will now have theirchat.updaterequests routed to the same host (https://my-proxy.example.com/slack/api/chat.update) instead of being forced back ontoslack.com. For these users this is the more correct behavior, but it is a behavior change and is recorded as a `[CHANGE]` in the CHANGELOG.Implementation
apiurl.Resolvercapturesapi_url/api_url_fileat notifier construction.URLForMethod("")returns the configured base URL (initial post);URLForMethod("chat.update")derives the URL by replacing the last path segment of the base, so any base path (e.g./slack/api/chat.postMessage) is honored.api_url_fileis read on every call, matching previous behavior so that file rotations apply to the next notification without restarting Alertmanager.notify/slack/slack.gonow delegates both URL constructions to the resolver. The previousTestGettingSlackURLFromFileandTestTrimmingSlackURLFromFileare replaced by the resolver-level coverage inapiurl_test.go.