Details

Reviewers

max
sashank

Commits

R1:25dd4aa33856: Support slackclient 2.x
R1:a785b00781d5: Support slackclient 2.x

Summary

As the title. With this change, we remove support for python 2.7.*, since slackclient 2.x is only supported for python 3.6 and above. See https://github.com/slackapi/python-slackclient/wiki/Migrating-to-2.x for details.

Addresses https://github.com/dagster-io/dagster/issues/3058

Test Plan

unit, BK

Diff Detail

Repository

R1 dagster

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rexledesma created this revision.Fri, Oct 16, 1:41 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Fri, Oct 16, 2:14 AM

Harbormaster failed remote builds in B19628: Diff 23838!

mock slack internal method when making api call

Harbormaster returned this revision to the author for changes because remote builds failed.Fri, Oct 16, 6:19 AM

Harbormaster failed remote builds in B19643: Diff 23853!

update buildkite

Harbormaster returned this revision to the author for changes because remote builds failed.Fri, Oct 16, 6:46 AM

Harbormaster failed remote builds in B19644: Diff 23854!

black

Harbormaster completed remote builds in B19645: Diff 23855.Fri, Oct 16, 7:07 AM

Harbormaster completed remote builds in B19646: Diff 23856.Fri, Oct 16, 7:09 AM

rexledesma requested review of this revision.Fri, Oct 16, 7:09 AM

rexledesma added reviewers: max, sashank.Fri, Oct 16, 3:44 PM

This is a breaking change, so we should think through this. We could consider doing a seven style import based on py2 vs py3

In D4797#129515, @sashank wrote:

This is a breaking change, so we should think through this. We could consider doing a seven style import based on py2 vs py3

Hm yeah I think you're right - the apis for slackclient1.3.2./slackclient2.** are pretty similar so this could work out

Taking a closer look, the new slack client is a pretty substantial refactor so it's probably not worth doing a seven style import. I think we may just want to bite the bullet and push a major breaking change to this library. If we do so, however, users who are stuck on py2 won't be able to use slack solid hooks etc.

@schrockn what do you think?

This revision now requires changes to proceed.Sun, Oct 18, 4:57 PM

I think we should cut over this library to be python 3 only and only support the new slack client and bite the bullet

It also seems tractable to test for the underlying client library version and call the correct API for an intermediate state through 0.10.0.

At minimum ping dwall about this

use conditional import

Harbormaster failed remote builds in B19891: Diff 24137!Tue, Oct 20, 10:45 PM

Harbormaster failed remote builds in B19895: Diff 24144!Tue, Oct 20, 11:22 PM

use python2 syntax :(

Harbormaster completed remote builds in B19900: Diff 24149.Wed, Oct 21, 12:11 AM

rexledesma retitled this revision from Update slackclient to >=2,<3 to Support slackclient 2.x when the python version is 3.x.Wed, Oct 21, 12:14 AM

rexledesma edited the summary of this revision. (Show Details)

i agree this should be python3 only, and we shouldn't attempt to support the old api. @schrockn dwall has effectively had to write his own dagster-slack library because of this.

This revision now requires changes to proceed.Wed, Oct 21, 3:50 PM

sashank added inline comments.Wed, Oct 21, 4:49 PM

python_modules/libraries/dagster-slack/dagster_slack/resources.py
6–7	Does this actually work generically? Or does it only work in our workspace? I have a feeling that you can't arbitrarily specify usernames and icons like this, and it only works in our workspace because we actually have a `dagsterbot` installed.

yeah @max i think i suggest this as a migration path to give people a deprecation warning before a hard breakage.

I'm willing to go either way. This doesn't seem too crazy, but we should probably warn

Looks like the consensus here is to just release it as a breaking change, without the conditional import. I guess the warning can be in the release notes lol

python_modules/libraries/dagster-slack/dagster_slack/resources.py
6–7	Ah, I just kept it since it was in the previous message arguments. But looks like `icon_url` can't be specified anyways with the newer tokens so I'll just remove it.