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.

Sign up for GitHub

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

Jump to bottom

add rate limiting for channel and user fetch #1115

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

add rate limiting for channel and user fetch #1115

wants to merge 16 commits into from

Conversation

sprutner
Copy link

No description provided.

@sprutner sprutner mentioned this pull request Aug 17, 2024
Open
DonDebonair
DonDebonair reviewed Aug 17, 2024
View reviewed changes
machine/clients/slack.py Outdated
@@ -93,6 +94,80 @@ async def _process_users_channels(self, _: AsyncBaseSocketModeClient, req: Socke
elif event["type"] == "member_joined_channel":
await self._on_member_joined_channel(event)

async def fetch_all_users(client):
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add type annotations for the function argument and return?

DonDebonair
DonDebonair requested changes Aug 17, 2024
View reviewed changes
Copy link
Owner

@DonDebonair DonDebonair left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @sprutner I assume you tested this on your workspace with many users?

I left a few comments to improve the code a bit. Great addition though! Excited to merge this in!

edit: also, please rebase on main. I merged some dependency upgrades. And while you're at it, I recommend running pre-commit install before committing the rebased code. As you can see, linting fails.

@sprutner
Copy link
Author

sprutner commented Aug 17, 2024 •
edited
Loading

Yeah I tested it on my workspace with like 68,000 users. It boots but it isn't respond to me when I DM or mention it..perhaps workspace permissions issue. I resubmitted the app to our workspace. I pushed a couple commits but haven't tested it. I never set the python env up it was just a late night hack. I can look more into cleaning this up over the weekend :)

@DonDebonair
Copy link
Owner

Yeah it would help if you can do a bit of serious testing :)

@sprutner
Copy link
Author

@DonDebonair doesn't seem like your tests run on each commit. It is hard for me to see if it's passing your tests here but re-requesting review. Installed the pre-commit hooks and ran them and got them to pass.

@sprutner
drop limit to 1000 and fix catch of ratelimited
2e61263 
slack sdk defaults to 1000 as the max
@DonDebonair
Copy link
Owner

Can you allow me to push to this branch on your fork? It's probably easier for me to fix the MyPy issues and I want to rebase the branch on main as well

dependabot bot added 11 commits August 19, 2024 10:37
@dependabot @sprutner
chore(deps-dev): bump ruff from 0.5.0 to 0.6.1
ae2cb67 
Bumps [ruff](https://github.com/astral-sh/ruff) from 0.5.0 to 0.6.1.
- [Release notes](https://github.com/astral-sh/ruff/releases)
- [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md)
- [Commits](astral-sh/ruff@0.5.0...0.6.1)

---
updated-dependencies:
- dependency-name: ruff
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @sprutner
chore(deps-dev): bump mkdocstrings-python from 1.10.3 to 1.10.5
1ae0dfc 
Bumps [mkdocstrings-python](https://github.com/mkdocstrings/python) from 1.10.3 to 1.10.5.
- [Release notes](https://github.com/mkdocstrings/python/releases)
- [Changelog](https://github.com/mkdocstrings/python/blob/main/CHANGELOG.md)
- [Commits](mkdocstrings/python@1.10.3...1.10.5)

---
updated-dependencies:
- dependency-name: mkdocstrings-python
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @sprutner
chore(deps): bump structlog from 24.2.0 to 24.4.0
745ec46 
Bumps [structlog](https://github.com/hynek/structlog) from 24.2.0 to 24.4.0.
- [Release notes](https://github.com/hynek/structlog/releases)
- [Changelog](https://github.com/hynek/structlog/blob/main/CHANGELOG.md)
- [Commits](hynek/structlog@24.2.0...24.4.0)

---
updated-dependencies:
- dependency-name: structlog
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @sprutner
chore(deps-dev): bump setuptools from 68.1.2 to 70.0.0
1e5a829 
Bumps [setuptools](https://github.com/pypa/setuptools) from 68.1.2 to 70.0.0.
- [Release notes](https://github.com/pypa/setuptools/releases)
- [Changelog](https://github.com/pypa/setuptools/blob/main/NEWS.rst)
- [Commits](pypa/setuptools@v68.1.2...v70.0.0)

---
updated-dependencies:
- dependency-name: setuptools
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @sprutner
chore(deps): bump aioboto3 from 13.1.0 to 13.1.1
3c3487e 
Bumps [aioboto3](https://github.com/terrycain/aioboto3) from 13.1.0 to 13.1.1.
- [Changelog](https://github.com/terricain/aioboto3/blob/main/CHANGELOG.rst)
- [Commits](terricain/aioboto3@v13.1.0...v13.1.1)

---
updated-dependencies:
- dependency-name: aioboto3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @sprutner
chore(deps-dev): bump zipp from 3.16.2 to 3.19.1
c3d9318 
Bumps [zipp](https://github.com/jaraco/zipp) from 3.16.2 to 3.19.1.
- [Release notes](https://github.com/jaraco/zipp/releases)
- [Changelog](https://github.com/jaraco/zipp/blob/main/NEWS.rst)
- [Commits](jaraco/zipp@v3.16.2...v3.19.1)

---
updated-dependencies:
- dependency-name: zipp
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @sprutner
chore(deps): bump certifi from 2023.7.22 to 2024.7.4
ee24119 
Bumps [certifi](https://github.com/certifi/python-certifi) from 2023.7.22 to 2024.7.4.
- [Commits](certifi/python-certifi@2023.07.22...2024.07.04)

---
updated-dependencies:
- dependency-name: certifi
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @sprutner
chore(deps): bump slack-sdk from 3.30.0 to 3.31.0
c946e2f 
Bumps [slack-sdk](https://github.com/slackapi/python-slack-sdk) from 3.30.0 to 3.31.0.
- [Release notes](https://github.com/slackapi/python-slack-sdk/releases)
- [Changelog](https://github.com/slackapi/python-slack-sdk/blob/main/docs-v2/changelog.html)
- [Commits](slackapi/python-slack-sdk@v3.30.0...v3.31.0)

---
updated-dependencies:
- dependency-name: slack-sdk
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @sprutner
chore(deps): bump aiohttp from 3.9.5 to 3.10.3
e362758 
Bumps [aiohttp](https://github.com/aio-libs/aiohttp) from 3.9.5 to 3.10.3.
- [Release notes](https://github.com/aio-libs/aiohttp/releases)
- [Changelog](https://github.com/aio-libs/aiohttp/blob/master/CHANGES.rst)
- [Commits](aio-libs/aiohttp@v3.9.5...v3.10.3)

---
updated-dependencies:
- dependency-name: aiohttp
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @sprutner
chore(deps-dev): bump coverage from 7.5.4 to 7.6.1
34af544 
Bumps [coverage](https://github.com/nedbat/coveragepy) from 7.5.4 to 7.6.1.
- [Release notes](https://github.com/nedbat/coveragepy/releases)
- [Changelog](https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst)
- [Commits](nedbat/coveragepy@7.5.4...7.6.1)

---
updated-dependencies:
- dependency-name: coverage
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @sprutner
chore(deps-dev): bump mkdocs-material from 9.5.28 to 9.5.31
64288ef 
Bumps [mkdocs-material](https://github.com/squidfunk/mkdocs-material) from 9.5.28 to 9.5.31.
- [Release notes](https://github.com/squidfunk/mkdocs-material/releases)
- [Changelog](https://github.com/squidfunk/mkdocs-material/blob/master/CHANGELOG)
- [Commits](squidfunk/mkdocs-material@9.5.28...9.5.31)

---
updated-dependencies:
- dependency-name: mkdocs-material
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
@sprutner
Copy link
Author

@DonDebonair done

@DonDebonair
Copy link
Owner

You let Dependabot update your branch, but those updates are also present on the main branch. This is causing conflicts when trying to rebase.

Can you fix your branch or open a new PR from the main branch (which is now fully up to date)

@DonDebonair
Copy link
Owner

@sprutner can you recreate the PR based on the latest main branch?

@DonDebonair DonDebonair mentioned this pull request Nov 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DonDebonair DonDebonair Awaiting requested review from DonDebonair

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sprutner @DonDebonair