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

fix(slack): replace slack GetChannels by GetConversations #1350

Merged
merged 1 commit into from
Jan 28, 2021

Conversation

thlacroix
Copy link
Contributor

fixes #1210

channels.list has been deprecated by slack and will break for existing applications in February 2021 (always breaks for new applications).

This PR replaces this call (used to check that the specified channel exists) by the new conversations.list which should by default work in a similar way.

It was needed to bump github.com/nlopes/slack as GetConversations was not available in the current version, but I decided not to go up to the latest as they changed the signature of PostMessage, which was trickier to mock in the tests.

@codecov
Copy link

codecov bot commented Jan 14, 2021

Codecov Report

Merging #1350 (debd858) into master (6b36adb) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1350      +/-   ##
==========================================
+ Coverage   70.01%   70.05%   +0.03%     
==========================================
  Files          74       74              
  Lines        5556     5563       +7     
==========================================
+ Hits         3890     3897       +7     
  Misses       1307     1307              
  Partials      359      359              
Impacted Files Coverage Δ
server/events/webhooks/slack_client.go 96.36% <100.00%> (+0.53%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b36adb...debd858. Read the comment docs.

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

LGTM

@thlacroix
Copy link
Contributor Author

Hi @lkysow , when do you think this can be released ?
In February all deployments which currently use the slack integration successfully will break and won't be able to start with their current configs (removing the webhook config would fix it though), so if this can be available by the end of the month that would be great.

@nishkrishnan nishkrishnan merged commit 6dfa6d5 into runatlantis:master Jan 28, 2021
@emre141
Copy link

emre141 commented Jan 28, 2021

@nishkrishnan Do you know about when would be able to new release that is cover this fix ?

@lkysow
Copy link
Member

lkysow commented Jan 29, 2021

we're going to try and get one out next week

@jmreicha
Copy link
Contributor

jmreicha commented Feb 17, 2021

Can somebody share a working configuration? I'm still getting initializing server: parsing /repos.yaml file: yaml: unmarshal errors: line 33: field webhooks not found in type raw.GlobalCfg using the following config in my repos.yaml config. I am setting the slack token via environment variable.

...
webhooks:
  - event: apply
    kind: slack
    channel: test-channel

@emre141
Copy link

emre141 commented Feb 17, 2021

@jmreicha I was used to deploy via helm as following, the link about configmap that is keeping following slack config

https://github.com/runatlantis/helm-charts/blob/main/charts/atlantis/templates/configmap-config.yaml

config: |
  slack-token: "{{ .Values.optiva_slack_token  }}"
  webhooks:
    - event: apply
      kind: slack
      channel: jpn-saasops-imp
      workspace-regex: optiva-inc.slack.com

@jmreicha
Copy link
Contributor

Ah ok, so it needs to be in a separate atlantis.yaml configuration then? That's the only difference I'm seeing.

@jmreicha
Copy link
Contributor

Splitting the configurations seems to work, erroring now with initializing server: initializing webhooks: missing_scope. Is there a list of minimum scopes needed? I have chat:write and incoming-webhook.

@jmreicha
Copy link
Contributor

jmreicha commented Feb 19, 2021

For anybody else that comes across the error, these permissions seem to be enough to fix it - channels:read, chat:write, groups:read, im:read, incoming-webhook, mpim:read.

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.

Slack configuration not working
5 participants