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

functions: Use Slack client lib for request validation #4369

Merged
merged 12 commits into from
Jul 29, 2020
Merged

Conversation

ace-n
Copy link

@ace-n ace-n commented Jul 24, 2020

N.B: this adds an additional package (slackclient) to do signature verification. (The code readability/maintainability benefits likely outweigh the drawbacks of adding an additional package.)

@ace-n ace-n requested review from tmatsuo and grant July 24, 2020 02:53
@ace-n ace-n requested a review from a team as a code owner July 24, 2020 02:53
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 24, 2020
Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

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

I really think we can't support Slack samples in our repos and we should move them to community samples. Adding slack dependencies like this isn't going to be long term sustainable with our other samples and languages.

Copy link
Contributor

@engelke engelke left a comment

Choose a reason for hiding this comment

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

First, fix lint errors.

Why is slackeventsapi in requirements.txt? It doesn't seem to be imported into any code. The corresponding Node.js sample uses a similarly named library in the signature verification (https://cloud.google.com/functions/docs/tutorials/slack#functions_slack_setup_tutorial-nodejs) instead of slackclient.

The prior version code does not seem too complex, and would show readers trying to use a different language the actual operations needed. Consider whether this change is good for all readers or not.

I don't feel strongly about whether to use the new slack library or not. So long as this sample is used in a tutorial on the website, the code does belong here.

@ace-n
Copy link
Author

ace-n commented Jul 25, 2020

@grant this tutorial was written quite awhile ago - likely before GSuite Chat was released. I'm potentially open to replacing it with one based around GSuite Chat.

@engelke I agree with your concern RE making this similar to other language samples. (Even if those languages have their own samples, this Python-specific implementation may come off as arbitrary.) Which would you (and @grant) say is more important here - consistency, or simpler code? (Personally I'm on the fence - I see value in both.)

@ace-n ace-n requested review from engelke and grant July 25, 2020 00:21
@ace-n
Copy link
Author

ace-n commented Jul 29, 2020

Ping on this. 🙂

@ace-n ace-n added the automerge Merge the pull request once unit tests and other checks pass. label Jul 29, 2020
Copy link
Contributor

@engelke engelke left a comment

Choose a reason for hiding this comment

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

LGTM

@engelke engelke merged commit 4a25177 into master Jul 29, 2020
@engelke engelke deleted the fix-1833 branch July 29, 2020 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the pull request once unit tests and other checks pass. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants