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

functions: fix slack + pubsub tests #3808

Merged
merged 12 commits into from
May 19, 2020
Merged

functions: fix slack + pubsub tests #3808

merged 12 commits into from
May 19, 2020

Conversation

ace-n
Copy link

@ace-n ace-n commented May 18, 2020

No description provided.

@ace-n ace-n requested a review from tmatsuo May 18, 2020 21:27
@ace-n ace-n requested review from grant and a team as code owners May 18, 2020 21:27
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 18, 2020
grant
grant approved these changes May 18, 2020
View reviewed changes
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.

Please address comment. Other than that LGTM.

functions/pubsub/main_test.py Show resolved Hide resolved
tmatsuo
tmatsuo reviewed May 18, 2020
View reviewed changes
Copy link
Contributor

@tmatsuo tmatsuo left a comment

Choose a reason for hiding this comment

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

@ace-n

Thanks, I assume you added slack secret in the encrypted file.
In that case, can you remove functions/slack/noxfile_config.py?

@ace-n
Copy link
Author

ace-n commented May 18, 2020

@tmatsuo yes - I've removed that file. 🙂

tmatsuo
tmatsuo reviewed May 18, 2020
View reviewed changes
Copy link
Contributor

@tmatsuo tmatsuo left a comment

Choose a reason for hiding this comment

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

@ace-n The tests are still failing FYI

functions/pubsub/main_test.py Show resolved Hide resolved
tmatsuo
tmatsuo approved these changes May 18, 2020
View reviewed changes
Copy link
Contributor

@tmatsuo tmatsuo 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.

LGTM modulo one question

functions/pubsub/main.py Outdated
@@ -62,7 +61,7 @@ def publish(request):

# [START functions_pubsub_subscribe]
# Triggered from a message on a Cloud Pub/Sub topic.
def subscribe(event, context):
def subscribe(event):
Copy link
Contributor

Choose a reason for hiding this comment

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

So why did we have context argument in the first place? Isn't there a case the functions service is sending the 2nd argument?

Copy link
Author

Choose a reason for hiding this comment

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

Update: GCF itself sends context as a second positional argument, so we cannot ignore it in the method signature.

tmatsuo
tmatsuo suggested changes May 19, 2020
View reviewed changes
functions/pubsub/main.py Outdated Show resolved Hide resolved
@tmatsuo
Copy link
Contributor

tmatsuo commented May 19, 2020

@ace-n
Thanks for the quick fix. I'll merge the master and then merge this PR.

@tmatsuo tmatsuo merged commit ab4166b into master May 19, 2020
@tmatsuo tmatsuo deleted the fix-3775 branch May 19, 2020 00:35
@tmatsuo tmatsuo mentioned this pull request May 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grant grant grant approved these changes

@tmatsuo tmatsuo tmatsuo approved these changes

Assignees

@gguuss gguuss

Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ace-n @tmatsuo @grant @gguuss @googlebot