Adopt existing channel on create #113

llamahunter · 2022-05-26T21:03:49Z

If a new 'adopt_existing_channel' attribute is set 'true' (default 'false'
for backwards compatibility), an existing channel with the same name
and type will be 'adopted' by the terraform provider, and unarchived
if necessary, to serve as the newly 'created' channel.
This brings parity with the existing provider functionality of archiving
or abandoning channels on terraform resource destroy. Now you can do
a 'terraform apply', 'terraform destroy', and 'terraform apply' again
without errors.

Fixes #112

llamahunter · 2022-05-26T21:09:55Z

go.mod

@@ -14,6 +14,7 @@ require (
 	github.com/hashicorp/go-getter v1.5.5 // indirect
 	github.com/hashicorp/go-multierror v1.1.1
 	github.com/hashicorp/hcl/v2 v2.10.0 // indirect
+	github.com/hashicorp/terraform-plugin-log v0.2.0 // indirect


using tflog for some diagnostic output on the new code I added. Probably should be more logging throughout?

llamahunter · 2022-05-26T21:18:07Z

slack/resource_conversation.go

@@ -107,6 +114,17 @@ func resourceSlackConversationCreate(ctx context.Context, d *schema.ResourceData
 	isPrivate := d.Get("is_private").(bool)

 	channel, err := client.CreateConversationContext(ctx, name, isPrivate)
+	if err != nil && err.Error() == "name_taken" && d.Get("adopt_existing_channel").(bool) {


if we get the 'name_taken' error, and the new 'adopt_exsting_channel' flag is true, then find the existing channel and use it instead of a newly created channel.

llamahunter · 2022-05-26T21:18:48Z

slack/resource_conversation.go

+			// ensure unarchived first if adopting existing channel, else other calls below will fail
+			if err := client.UnArchiveConversationContext(ctx, channel.ID); err != nil {


if the channel is currently archived, even if it is configured to eventually be archived, we need to unarchive it as many of the calls below will fail if called on an archived channel.

NOTE! this api does not work with bot tokens (despite documentation to the contrary). The provider must be configured with a user token.

llamahunter · 2022-05-26T21:20:30Z

slack/resource_conversation.go

+				if err.Error() != "not_archived" {
+					return diag.Errorf("couldn't unarchive conversation %s: %s", channel.ID, err)


we really should never get this error, as we check that it's archived before unarchived, but I guess there could be race conditions with a user unarchiving it manually in between the two calls.

llamahunter · 2022-05-26T21:21:43Z

slack/resource_conversation.go

+	// find the existing channel. Sadly, there is no non-admin API to search by name,
+	// so we must search through ALL the channels


An extreme sadness with the existing slack api. There is an admin api for searching for a specific channel, but that would greatly limit the deployment ability of this provider, as it would need to be configured with an administrator user token.

llamahunter · 2022-05-26T21:23:32Z

slack/resource_conversation.go

+			if rateLimitedError, ok := err.(*slack.RateLimitedError); ok {
+				tflog.Warn(ctx, "rate limited for %f seconds", rateLimitedError.RetryAfter.Seconds())


some slack workspaces have thousands of channels, so even going 200 at a time will possibly hit rate limiting. So we need to handle waiting and retrying.

llamahunter · 2022-05-26T21:24:49Z

slack/resource_conversation.go

+				select {
+				case <-ctx.Done():
+					return nil, fmt.Errorf("canceled during pagination: %s", ctx.Err())
+				case <-time.After(rateLimitedError.RetryAfter):
+					tflog.Debug(ctx, "done sleeping after rate limited")
+				}


This is similarly implemented to the internals of the paginated GetUsersContext in the slack-go library that this provider is built upon.
https://github.com/slack-go/slack/blob/34c7fd379e3526317abcc057ef7baa686cb0a42c/users.go#L379

llamahunter · 2022-05-26T21:25:24Z

slack/resource_conversation.go

+			for _, c := range channels {
+				tflog.Trace(ctx, "current channel %s", c.Name)
+				if c.Name == name {


check every single channel, but bail as soon as we find a match

llamahunter · 2022-05-26T21:25:56Z

slack/resource_conversation.go

@@ -246,7 +312,7 @@ func resourceSlackConversationUpdate(ctx context.Context, d *schema.ResourceData
 		} else {
 			if err := client.UnArchiveConversationContext(ctx, id); err != nil {
 				if err.Error() != "not_archived" {
-					return diag.Errorf("couldn't archive conversation %s: %s", id, err)
+					return diag.Errorf("couldn't unarchive conversation %s: %s", id, err)


this is an unarchive, so the error was inaccurate.

llamahunter · 2022-05-26T21:26:19Z

slack/resource_conversation.go

@@ -246,7 +312,7 @@ func resourceSlackConversationUpdate(ctx context.Context, d *schema.ResourceData
 		} else {
 			if err := client.UnArchiveConversationContext(ctx, id); err != nil {


Note as above that this call does NOT work with bot tokens, only user tokens. :-(

pablovarela · 2022-06-13T11:53:15Z

Thank you very much for your contribution! And apologies for the late response. I've been quite busy and neglected this project.

The change looks fine. In my view There are two things lacking: tests and documentation.

I would ask you to at least document the new parameter adopt_existing_channel in the conversation resource documentation (docs/resources/conversation.md), and make it very clear that user tokens are required to make it work properly.

Could you also update the PR with the latest changes in master? There are conflicts that can not be automerged. I will allow linters and tests to run for this PR.

Once that is done I will happily merge it and release a new version.

llamahunter · 2022-06-13T16:19:31Z

ok, thx! Will look into docs and whatever tests I can figure out. I actually wasn't sure how to test this, since it seems to want credentials for a slack account and I can't have it run its tests on our corporate account.

pablovarela · 2022-06-13T17:15:00Z

Oh I see. Yes, you need a slack token to run the tests, set using SLACK_TOKEN env variable. When you push the changes they should run here, if not I can run them locally for you. Not ideal but better than nothing.

llamahunter · 2022-07-13T00:59:33Z

docs/resources/conversation.md

 - [channels:read](https://api.slack.com/scopes/channels:read) (public channels)
 - [channels:manage](https://api.slack.com/scopes/channels:manage) (public channels)
+- [channels:join](https://api.slack.com/scopes/channels:join) (adopting existing public channels)


bot token separates 'join' permission from 'manage'

llamahunter · 2022-07-13T01:00:00Z

docs/resources/conversation.md

+
+If using `user` tokens:
+- [channels:read](https://api.slack.com/scopes/channels:read) (public channels)
+- [channels:write](https://api.slack.com/scopes/channels:manage) (public channels)


user token uses 'write' rather than 'manage', and 'write' includes join permission already.

llamahunter · 2022-07-13T01:00:13Z

docs/resources/conversation.md

 - [groups:read](https://api.slack.com/scopes/groups:read) (private channels)
 - [groups:write](https://api.slack.com/scopes/groups:write) (private channels)

 The Slack API methods used by the resource are:

 - [conversations.create](https://api.slack.com/methods/conversations.create)
+- [conversations.join](https://api.slack.com/methods/conversations.join)


we need to be able to join existing unarchived channels

llamahunter · 2022-07-13T01:00:27Z

docs/resources/conversation.md

+```hcl
+resource "slack_conversation" "adopted" {
+  name                   = "my-channel02"
+  topic                  = "If already existing, channel will be adopted, and existing users not kicked"
+  permanent_members      = []
+  adopt_existing_channel = true
+  remove_non_members     = false
+}
+```


An example of the two new parameters in use

llamahunter · 2022-07-13T01:01:33Z

docs/resources/conversation.md

 `archive | none`. Note that when set to `none` the conversation will be left
 as it is  and as a result any subsequent runs of terraform apply with the same
 name  will fail.


Note that I think, regardless of whether archive or none is specified, subsequent applies will fail unless adopt_existing_channel is set to true.

llamahunter · 2022-07-13T01:01:42Z

docs/resources/conversation.md

+- `adopt_existing_channel` (Optional, Default `false`) indicates that an existing 
+channel with the same name should be adopted by terraform and put under state
+management. If the existing channel is archived, it will be unarchived. (Note: for 
+unarchiving of existing channels to work correctly, you _must_ use a user token,


Added the note about the need for 'user' tokens

llamahunter · 2022-07-13T01:02:20Z

docs/resources/conversation.md

+- `remove_non_members` (Optional, Default `true`) indicates that members of the
+channel that aren't listed in the `permament_members` array will be kicked. (Note:
+your Slack configuration may restrict/prohibit kicking of members from channels
+except when using administrator credentials)


this is especially needed for adopting existing channels, as permission to remove non members may be denied by slack configuration.

llamahunter · 2022-07-15T21:30:10Z

@pablovarela I added docs, and also added another needed parameter to make this work on slack servers that have restricted 'kick' permissions.
Still not clear how to test this w/o a specially designated slack server.

pablovarela · 2022-07-26T16:01:27Z

The remove_non_members option has been superseded by #126. and it is now part of v1.1.21 See documentation here.

If you could remove remove_non_members and update the docs I will test it locally and merge this PR asap. I will keep an eye in it this week.

llamahunter · 2022-07-26T16:05:33Z

Ok, I saw that other PR, but looked like it wasn't going anywhere, and this was critically needed to make the adopting of existing channels work correctly. Will re-base on master and resubmit the PR sometime later today.

llamahunter · 2022-07-27T00:15:05Z

@pablovarela rebased on master, removing remove_non_members flag.

llamahunter · 2022-07-29T18:32:53Z

@pablovarela any chance this could get merged soon?

llamahunter · 2022-07-29T18:51:22Z

@pablovarela I see there was another PR that jumped ahead of this one, causing merge conflicts again. Rebased again. Please review and merge ASAP.

llamahunter · 2022-08-01T16:45:37Z

@pablovarela ping

pablovarela · 2022-08-02T09:52:42Z

on it

pablovarela

There are some linting issues (a magic number and some markdown ones), and if I ignore linting the tests are not passing. You can run the tests locally using make.

I will provide you with a token that I will change later, so you can test. Please set the environment variable SLACK_TOKEN=xoxp-1437037581287-1448689260021-1448696433877-1e561acc1efe312956ea7b00d629f805 and run the tests

llamahunter · 2022-08-02T14:58:05Z

slack/resource_conversation.go

+	for !paginationComplete {
+		channels, nextCursor, err := client.GetConversationsContext(ctx, &slack.GetConversationsParameters{
+			Cursor: cursor,
+			Limit:  200, // 100 is default, docs recommend no more than 200, but 1000 is the max


this magic number is from the documentation of the slack api. See, for instance, a similar usage in their golang library.
https://github.com/slack-go/slack/blob/master/users.go#L305

llamahunter · 2022-08-05T19:42:04Z

made more changes to satisfy linter, at cost of making documentation harder to read in edit mode.

llamahunter · 2022-08-05T19:42:37Z

@pablovarela

pablovarela

The linter is still complaining. If you run make docs-lint-fix it will fix the remaining linter issues.

I run that locally, and the linter passes. But the tests still fails, for example

=== CONT  TestAccSlackConversationTest/add_permanent_members
    resource_conversation_test.go:162: Step 1/3 error: Error running apply: exit status 1
        
        Error: api user could not join conversation: method_not_supported_for_channel_type
        
          with slack_conversation.test,
          on terraform_plugin_test.tf line 2, in resource "slack_conversation" "test":
           2: resource slack_conversation test {

If a new 'adopt_existing_channel' attribute is set 'true' (default 'false' for backwards compatibility), an existing channel with the same name and type will be 'adopted' by the terraform provider, and unarchived if necessary, to serve as the newly 'created' channel. This brings parity with the existing provider functionality of archiving or abandoning channels on terraform resource destroy. Now you can do a 'terraform apply', 'terraform destroy', and 'terraform apply' again without errors.

membership operations (needed when adopting existing unarchived channels)

The upstream provider updated the sdk version, which pulled in this new version. The new version has breaking API changes to logging to make it more 'structured'.

- some channel types (notably private ones) can't be joined. - ephemeral attributes shouldn't be checked

pablovarela · 2022-08-20T14:34:39Z

This is not part of 1.1.24 release.

Thank you so much for your contribution and your patience.

llamahunter · 2022-08-23T18:49:23Z

This is not part of 1.1.24 release.

Thank you so much for your contribution and your patience.

Thx!

This is not part of 1.1.24? It looks like it's in the release notes, tho.

pablovarela · 2022-08-30T02:09:52Z

it is part of 1.1.24

commit 9d760fc9c0bca9885ce402e0a609fa68599f58b7 (tag: v1.1.24)
Author: Richard Lee <[email protected]>
Date:   Sat Aug 20 07:16:26 2022 -0700

    Adopt existing channel on create (#113)

    * Adopt existing channel on create
    If a new 'adopt_existing_channel' attribute is set 'true' (default 'false'
    for backwards compatibility), an existing channel with the same name
    and type will be 'adopted' by the terraform provider, and unarchived
    if necessary, to serve as the newly 'created' channel.
    This brings parity with the existing provider functionality of archiving
    or abandoning channels on terraform resource destroy. Now you can do
    a 'terraform apply', 'terraform destroy', and 'terraform apply' again
    without errors.

    * Make sure api user is in the channel before doing channel
    membership operations (needed when adopting existing unarchived
    channels)

    * Update to terraform-plugin-log v0.4.0
    The upstream provider updated the sdk version, which pulled in this
    new version. The new version has breaking API changes to logging to
    make it more 'structured'.

    * Update documentation on conversation api.

    * Update go.mod and go.sum for new deps

    * Update go.mod to version 0.7.0 of terraform-plugin-log

    * added newline

    * Made changes to satisfy pedantic linter.

    * Address pedantic docs linter whitespace issues.

    * Fix test failures
    - some channel types (notably private ones) can't be joined.
    - ephemeral attributes shouldn't be checked

llamahunter commented May 26, 2022

View reviewed changes

llamahunter mentioned this pull request May 26, 2022

Cannot unarchive channel previously destroyed via 'action_on_destroy' = 'archive' #112

Closed

llamahunter force-pushed the unarchive-channel-on-create branch from 05f9b93 to 1ffdcc6 Compare July 11, 2022 19:36

llamahunter commented Jul 13, 2022

View reviewed changes

llamahunter force-pushed the unarchive-channel-on-create branch from 4299839 to 5290d3f Compare July 27, 2022 00:12

llamahunter force-pushed the unarchive-channel-on-create branch from 5290d3f to f137252 Compare July 29, 2022 18:50

pablovarela requested changes Aug 2, 2022

View reviewed changes

llamahunter commented Aug 2, 2022

View reviewed changes

llamahunter requested a review from pablovarela August 9, 2022 16:18

pablovarela requested changes Aug 9, 2022

View reviewed changes

llamahunter added 7 commits August 19, 2022 17:24

Make sure api user is in the channel before doing channel

d700158

membership operations (needed when adopting existing unarchived channels)

Update to terraform-plugin-log v0.4.0

8113c26

The upstream provider updated the sdk version, which pulled in this new version. The new version has breaking API changes to logging to make it more 'structured'.

Update documentation on conversation api.

14703a3

Update go.mod and go.sum for new deps

6915184

Update go.mod to version 0.7.0 of terraform-plugin-log

2def2ed

added newline

3e0c835

llamahunter added 2 commits August 19, 2022 17:28

Made changes to satisfy pedantic linter.

c660df9

Address pedantic docs linter whitespace issues.

e6cf23a

llamahunter force-pushed the unarchive-channel-on-create branch from a8fcfa6 to e6cf23a Compare August 20, 2022 00:28

Fix test failures

7f98cda

- some channel types (notably private ones) can't be joined. - ephemeral attributes shouldn't be checked

pablovarela merged commit 9d760fc into pablovarela:master Aug 20, 2022

llamahunter deleted the unarchive-channel-on-create branch August 29, 2022 17:57

		// ensure unarchived first if adopting existing channel, else other calls below will fail
		if err := client.UnArchiveConversationContext(ctx, channel.ID); err != nil {

		if err.Error() != "not_archived" {
		return diag.Errorf("couldn't unarchive conversation %s: %s", channel.ID, err)

		// find the existing channel. Sadly, there is no non-admin API to search by name,
		// so we must search through ALL the channels

		if rateLimitedError, ok := err.(*slack.RateLimitedError); ok {
		tflog.Warn(ctx, "rate limited for %f seconds", rateLimitedError.RetryAfter.Seconds())

		@@ -246,7 +312,7 @@ func resourceSlackConversationUpdate(ctx context.Context, d *schema.ResourceData
		} else {
		if err := client.UnArchiveConversationContext(ctx, id); err != nil {

Adopt existing channel on create #113

Adopt existing channel on create #113

Conversation

llamahunter commented May 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llamahunter May 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablovarela commented Jun 13, 2022 • edited Loading

llamahunter commented Jun 13, 2022

pablovarela commented Jun 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llamahunter commented Jul 15, 2022

pablovarela commented Jul 26, 2022 • edited Loading

llamahunter commented Jul 26, 2022

llamahunter commented Jul 27, 2022

llamahunter commented Jul 29, 2022

llamahunter commented Jul 29, 2022

llamahunter commented Aug 1, 2022

pablovarela commented Aug 2, 2022

pablovarela left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llamahunter commented Aug 5, 2022

llamahunter commented Aug 5, 2022

pablovarela left a comment

Choose a reason for hiding this comment

pablovarela commented Aug 20, 2022

llamahunter commented Aug 23, 2022

pablovarela commented Aug 30, 2022 • edited Loading

llamahunter May 26, 2022 •

edited

Loading

pablovarela commented Jun 13, 2022 •

edited

Loading

pablovarela commented Jul 26, 2022 •

edited

Loading

pablovarela left a comment •

edited

Loading

pablovarela commented Aug 30, 2022 •

edited

Loading