Slack #170
Merged
Slack #170
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
christopher-schroeder
requested a review
from johanneskoester
as a code owner
johanneskoester
requested changes
Jan 9, 2020
There was a problem hiding this comment.
Almost missed it: can you fix the three code smells? See
https://sonarcloud.io/project/issues?id=snakemake_snakemake&pullRequest=170&resolved=false&types=CODE_SMELL
|
Kudos, SonarCloud Quality Gate passed!
|
johanneskoester
added a commit
that referenced
this pull request
Dec 13, 2023
### Description <!--Add a description of your PR here--> This updates the Slack ``--log-service`` feature added in #170. The ``Slacker`` package is no longer maintained, and legacy slack tokens are no longer generatable. I've replace it with the officially supported [Slack SDK](https://github.com/slackapi/python-slack-sdk) (see ``setup.cfg``), preserving the original functionality with minimal adjustments. This has been minimally tested for functionality. However, the original implementation has a couple issues (which this PR **does not** address) - 1. Lack of `docs/` on how to set up the ``--log-service slack`` feature via Slack 2. Logic is not robust - e.g. when `msg["done"] == msg["total"]`, this does not mean the workflow is complete (checkpoints) 3. The implementation of ``--log-service slack`` is synchronous, meaning that Snakemake workflows will wait for the implemented `log_handler` function to finish. For normal print statements, this isn't an issue, but repeated calls of Slack WebAPI's `postMessage` can significantly slow things down. Depending on the workflow (Snakefile), this can become noticeable. 4. The implementation does not comply with Slack WebAPI's rate limiting. The current implementation will look like this - messages on slack are sent by the user, using a user OAuth token.  I've already addressed points 2, 3, and 4 in a private repository using ``--log-handler`` that should be integrable into this current implementation, but it has not been robustly tested (integration can happen at a later PR).  Thanks! ### QC <!-- Make sure that you can tick the boxes below. --> * [ ] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [ ] The documentation (`docs/`) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake). --------- Co-authored-by: Johannes Köster <johannes.koester@tu-dortmund.de>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows snakemake to use an exported slack legacy token to send the user a message on successfully executed and failed workflows. It is written in a way that additional messaging services can be added in near future.
The creators of slack recommend to not use legacy tokens anymore, but their newly provided slack app system. This provides indeed many additional features, but as far as I can see this would require the user to configure the channel for a snakemake communication. Since there would be no improvement in security and the task is to only send simple messages to the executing user, in my opinion the legacy tokens are sufficient.