Page MenuHomePhabricator

Support slackclient 2.x
ClosedPublic

Authored by rexledesma on Fri, Oct 16, 1:41 AM.

Details

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

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!
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!

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

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

sashank requested changes to this revision.Sun, Oct 18, 4:57 PM
sashank added a subscriber: schrockn.

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

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)
max requested changes to this revision.Wed, Oct 21, 3:50 PM

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
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.

max requested changes to this revision.Thu, Oct 22, 3:42 PM
max added inline comments.
python_modules/libraries/dagster-slack/dagster_slack/resources.py
23

we should drop this whole wrapper class and just expose the client

This revision now requires changes to proceed.Thu, Oct 22, 3:42 PM
python_modules/libraries/dagster-slack/dagster_slack/resources.py
23

whynot

max added inline comments.
python_modules/libraries/dagster-slack/setup.py
24

also 3.8

30–31

this library doesn't support python 2o so that's fine, remove this comment

rexledesma retitled this revision from Support slackclient 2.x when the python version is 3.x to Support slackclient 2.x.Thu, Oct 22, 8:49 PM
rexledesma edited the summary of this revision. (Show Details)
This revision was not accepted when it landed; it landed in state Needs Review.Thu, Oct 22, 8:50 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.