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: move to slack-sdk files_upload_v2 #28423

Merged
merged 3 commits into from
May 15, 2024
Merged

fix: move to slack-sdk files_upload_v2 #28423

merged 3 commits into from
May 15, 2024

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented May 10, 2024

Randomly caught #28353 and decided to propose a fix. This seemed straightforward when I looked at the docs here https://github.com/slackapi/python-slack-sdk/releases/tag/v3.19.0

Confirmed we're on the latest client and that stuff is pretty well covered by unit tests

pyproject.toml Outdated
@@ -85,7 +85,7 @@ dependencies = [
"shortid",
"sshtunnel>=0.4.0, <0.5",
"simplejson>=3.15.0",
"slack_sdk>=3.19.0, <4",
"slack_sdk>=3.19.0",
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the upper bound? Typically we try to bind dependencies to their current major version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's talk about this! I wanted to start by noting that those boundaries are used by raw "pip install apache-superset" (where having an upper bound is probably a good idea, fully pinned for that use case might be an even better approach...), and by @supersetbot to auto-generate PRs bumping pinned libs, within the boundaries set here.

First question is what should happen when people in the wild just go pip install apache-superset, all of our installation docs / docker / CI is pretty clear about running the pinned dep installs first as in pip install -r requirements/base.txt FIRST to get to something that's actually likely to work. Now knowing that people may not do that, should we 1. pin stuff in the package ? or 2. identify ranges of what's likely to work (kind of what we have now with semver upper bounds). Note that the approach of "what's likely to work" ranges is fairly subjective, and there's a fair amount of things that can go wrong with this approach. Say if the cryptography lib has a major fix I NEED in semver+1, the semver package upper bound restriction is actually preventing me from getting what I need. All sorts of other non-pinned stuff can still go wrong.

One thing is I think we need to differentiate semantics around real breaking bounds (as in "we know library >=N.x won't work!") and hopeful ones (as in "given what we know about semver, library > N.x could have breaking changes...").

Anyhow, that's a complex topic, let's chat.

@mistercrunch
Copy link
Member Author

For now let me remove that line

Randomly caught #28353 and
decided to propose a fix. Unclear if this is unit tested properly or not
but thought I'd put it out there as it seemed straightforward when I
looked at the docs here
https://github.com/slackapi/python-slack-sdk/releases/tag/v3.19.0
@mistercrunch mistercrunch merged commit 97341a1 into master May 15, 2024
30 checks passed
@mistercrunch mistercrunch deleted the slack_v2 branch May 15, 2024 21:32
jzhao62 pushed a commit to jzhao62/superset that referenced this pull request May 16, 2024
@eschutho eschutho mentioned this pull request May 30, 2024
9 tasks
EnxDev pushed a commit to EnxDev/superset that referenced this pull request May 31, 2024
eschutho pushed a commit that referenced this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants