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

add handler for file uploads in slack connector, update async test runner plugin, update documentation parser #2020

Merged

Conversation

pmaresca
Copy link
Contributor

@pmaresca pmaresca commented Mar 26, 2024

Description

Provides a handler in the slack connector per @jacobtomlinson's recommmendation in #1611
Slightly updated as the api available after recommendations has been updated.
I also have taken a shot at replacing the async test runner for the project with anyio.

I had to add an additional parameter to OpsDroid init to allow bypassing initialization of the eventloop to avoid chicken/egg problems with initializing the anyio task group in certain sync test cases i.e. test_configration.py

Fixes #1611

Status

UNDER DEVELOPMENT

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update
  • Documentation (fix or adds documentation)

How Has This Been Tested?

I added a test case but still trying to figure out running the suite

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to raise this.

This CI is not in a good way here, but unfortunately I don't have time to dig into it. I think before we can merge this someone needs to take a look at it.

@pmaresca
Copy link
Contributor Author

pmaresca commented Apr 3, 2024

Of course, thanks for the project proper :).
I'm not really up to speed on who's maintaining this stuff actively but thanks for looking, hopefully another maintainer will swing by.

@jacobtomlinson
Copy link
Member

Currently nobody is very active here. If you have any interest into digging into this it would be much appreciated.

@pmaresca
Copy link
Contributor Author

pmaresca commented Apr 4, 2024

Well I would like file uploads... I will have to find some time to tinker with the test suite.
Unfortunately I'm totally new to using tox so I have a bit of reading to do I think.

@pmaresca
Copy link
Contributor Author

pmaresca commented Apr 16, 2024

@jacobtomlinson So I think I figured out why the test suite is bombing out completely(or at least narrowed it down)
Something in the latest minor of pytest-asyncio breaks the pytest suite. pinning it to <0.22.0 allows the test suite to run again.
However now that I've figured that out I've uncovered some more info about slack file uploads....
https://github.com/slackapi/python-slack-sdk/releases/tag/v3.19.0
So it seems that the old way of file uploading is going away. There's this new three endpoint dance you have to do to upload the file...great.


   1. Call WebClient#files_getUploadURLExternal() method to receive a URL to use for each file
   2. Perform an HTTP POST request to the URL you received in step 1 for each file
   3. Call WebClient#files_completeUploadExternal() method with the pairs of file ID and title to complete the whole process, plus share the files in a channel

they've provided a wrapper method for this whole dance called files_upload_v2
So I guess my question at this point is what is your expectation here, should I be adding the explicit calls for each step(plus tests) to perform the file upload? Or if I can utilize the wrapper, would it be sufficient to create a mock for the method and assert the parameters are correct?

@pmaresca
Copy link
Contributor Author

So I went through the bother of setting up a test workspace for slack and I'm running my opsdroid build from this branch and it seems like the v2 file upload thing works, at least for simple text snippets. I'm still trying to figure out how to get the thread reply working though
Screenshot 2024-04-16 at 1 45 16 AM

@jacobtomlinson
Copy link
Member

In many projects I've switched from pytest-asyncio to pytest-anyio and just disabled the trio backend. It should just be a case of changing which package is installed and then adding a fixture to configure the backend.

@pmaresca
Copy link
Contributor Author

haven't heard of anyio before...more reading :)

@pmaresca pmaresca changed the title add handler for file uploads in slack connector add handler for file uploads in slack connector, update async test runner plugin Apr 20, 2024
@pmaresca
Copy link
Contributor Author

@jacobtomlinson so the anyio swap was a little more work than adding a fixture but...ci is almost all green.
I am not sure why the windows builds don't pass yet though unfortunately.

I made some decisions to avoid fully reworking all the event loop references in the project into anyio friendly code for the sake of trying to keep this PR to a reasonable size, but I did have to do something to stop the yelling about accessing the event loop.

This probably still needs a bit of cleanup and I guess I also have docs to update too now assuming this is a step in the right direction

@jacobtomlinson
Copy link
Member

Thanks for taking the time and pushing things forwards here.

I'm also not sure what is going on with the Windows failures.

For the docs builds it looks like the config for ReadTheDocs is out of date. Hopefully it should be a small config tweak to get it working again.

@pmaresca
Copy link
Contributor Author

Read the docs stuff was deprecated, what a surprise :)... Recommonmark project recomends transitioning to myst parser so...I transitioned to myst parser. There may be some slight differences I haven't noticed but the latest _build output I looked at seemed alright.

@pmaresca
Copy link
Contributor Author

Alright read the docs is green

@pmaresca
Copy link
Contributor Author

🥳

@pmaresca pmaresca changed the title add handler for file uploads in slack connector, update async test runner plugin add handler for file uploads in slack connector, update async test runner plugin, update documentation parser Apr 24, 2024
Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Wow! Thank you so much for all your efforts here. If you DM me your address on Twitter I'll post you some opsdroid stickers to thank you for perservering here!

@jacobtomlinson jacobtomlinson merged commit 9b6c9e6 into opsdroid:master Apr 25, 2024
14 checks passed
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.

Feature Request: Please consider implementing 'snippet' support for the Slack connector
2 participants