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

Integrated Slack Action #88

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

rpartha
Copy link

@rpartha rpartha commented Oct 5, 2019

Created the Slack action based on existing template(s). Should work, however, token not stored as environment config and Slack message API response was not checked (asserted).

@coveralls
Copy link

coveralls commented Oct 5, 2019

Pull Request Test Coverage Report for Build 108

  • 12 of 27 (44.44%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.3%) to 56.578%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pastepwn/actions/slackaction.py 10 25 40.0%
Totals Coverage Status
Change from base Build 82: -0.3%
Covered Lines: 701
Relevant Lines: 1239

💛 - Coveralls

@d-Rickyy-b
Copy link
Owner

Hi there :) Thank you for your work! Could you please check, why your PR does not work on Python 3.4 and 3.5? Is there no slack package for those versions?

@rpartha
Copy link
Author

rpartha commented Oct 5, 2019

The slack sdk (current v2) was only built for Python 3.6+, while v1 of the sdk supports Python 2.x, according to the official docs

@d-Rickyy-b
Copy link
Owner

d-Rickyy-b commented Oct 5, 2019

The slack sdk (current v2) was only built for Python 3.6+, while v1 of the sdk supports Python 2.x, according to the official docs

Would there be any possibility to try-except the imports so that it uses another sdk depending on the runtime environment? I am thinking of something similar to this:

https://github.com/python-telegram-bot/python-telegram-bot/blob/4689a80c2e623a03976b7e5b878d358639396690/telegram/base.py#L21-L24

That would be pretty awesome, because I don't want to drop support for Python 3.4 yet. I know it's EOL already, but to me it makes sense supporting it for a few months.

EDIT: And afaik 3.5 is not EOL yet, so it would be nice to support that at least.

Would be very cool if you could check if it works as I suggested and if yes, implement it. If it does not work, could you please raise an error on init?

Thank you very much :)

@rpartha
Copy link
Author

rpartha commented Oct 5, 2019

Would be very cool if you could check if it works as I suggested and if yes, implement it

I can use the v1 (2.x) API if v2 (3.6.x+) raises an error, like the link you posted, but it still wouldn't work for 3.4 as there it isn't supported by API.

else:
paste_dict = paste.to_dict()
paste_dict["analyzer_name"] = analyzer_name
text = self.template.safe_substitute(DictWrapper(paste_dict))
Copy link
Owner

Choose a reason for hiding this comment

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

Big plus for the templating :) thank you very much,

pastepwn/actions/slackaction.py Show resolved Hide resolved
@d-Rickyy-b
Copy link
Owner

Would be very cool if you could check if it works as I suggested and if yes, implement it

I can use the v1 (2.x) API if v2 (3.6.x+) raises an error, like the link you posted, but there still wouldn't be any support for 3.4 as there isn't any support for it.

Alright, thanks for getting back. I will think about how to handle it in a nice way.

@d-Rickyy-b d-Rickyy-b mentioned this pull request Oct 6, 2019
@felurx
Copy link

felurx commented May 18, 2022

Time has passed, and the README says that anything below 3.6 isn't supported anyways, so I suppose the problem with 3.4/3.5 is out of the way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants