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.
Details
Details
unit, BK
Diff Detail
Diff Detail
- Repository
- R1 dagster
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
This is a breaking change, so we should think through this. We could consider doing a seven style import based on py2 vs py3
Comment Actions
Hm yeah I think you're right - the apis for slackclient1.3.2./slackclient2.** are pretty similar so this could work out
Comment Actions
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?
Comment Actions
I think we should cut over this library to be python 3 only and only support the new slack client and bite the bullet
Comment Actions
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
Comment Actions
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.
| python_modules/libraries/dagster-slack/dagster_slack/resources.py | ||
|---|---|---|
| 7–8 | 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. | |
Comment Actions
yeah @max i think i suggest this as a migration path to give people a deprecation warning before a hard breakage.
Comment Actions
I'm willing to go either way. This doesn't seem too crazy, but we should probably warn
Comment Actions
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 | ||
|---|---|---|
| 7–8 | 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. | |
| python_modules/libraries/dagster-slack/dagster_slack/resources.py | ||
|---|---|---|
| 23 | we should drop this whole wrapper class and just expose the client | |
| python_modules/libraries/dagster-slack/dagster_slack/resources.py | ||
|---|---|---|
| 23 |
| |
