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

fixes #33: Only listen to functionTarget. #81

Merged
merged 5 commits into from
Sep 6, 2019
Merged

fixes #33: Only listen to functionTarget. #81

merged 5 commits into from
Sep 6, 2019

Conversation

grant
Copy link
Contributor

@grant grant commented Sep 2, 2019 •
edited
Loading

@grant grant requested a review from hdp617 September 2, 2019 20:07
@grant grant self-assigned this Sep 2, 2019
@hdp617
Copy link
Contributor

hdp617 commented Sep 3, 2019

Could you also update the test?

@grant
Copy link
Contributor Author

grant commented Sep 3, 2019 •
edited
Loading

Could you also update the test?

@hdp617 Addressed comment and fixed Travis CI tests.

PTAL.

hdp617
hdp617 reviewed Sep 5, 2019
View reviewed changes
test/invoker.ts Outdated
@@ -34,7 +34,8 @@ describe('request to HTTP function', () => {
(req: express.Request, res: express.Response) => {
res.send(req.body.text.toUpperCase());
},
invoker.SignatureType.HTTP
invoker.SignatureType.HTTP,
'/'
Copy link
Contributor

@hdp617 hdp617 Sep 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is an odd function target. I suggest something like 'testFunction'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied suggestion of testFunction.

hdp617
hdp617 reviewed Sep 5, 2019
View reviewed changes
test/invoker.ts Outdated
@@ -34,7 +34,8 @@ describe('request to HTTP function', () => {
(req: express.Request, res: express.Response) => {
res.send(req.body.text.toUpperCase());
},
invoker.SignatureType.HTTP
invoker.SignatureType.HTTP,
'/'
);
return supertest(server)
.post('/')
Copy link
Contributor

Choose a reason for hiding this comment

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

.post('/'testFunction')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

hdp617
hdp617 reviewed Sep 5, 2019
View reviewed changes
Copy link
Contributor

@hdp617 hdp617 left a comment

Choose a reason for hiding this comment

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

Question: This is a breaking change since we no longer invoke the function with /*. Any GCF users relying on this?

grant
grant commented Sep 6, 2019
View reviewed changes
Copy link
Contributor Author

@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 applied the suggestion of using a more useful path for the routing tests.

Question: This is a breaking change since we no longer invoke the function with /*. Any GCF users relying on this?

I don't believe GCF users are relying on this as its not possible to invoke the function at other URLs besides the functionTarget GCF URL.

This should still be a major change as a request to /favicon.ico will not invoke the function anymore (see original issue).

test/invoker.ts Outdated
@@ -34,7 +34,8 @@ describe('request to HTTP function', () => {
(req: express.Request, res: express.Response) => {
res.send(req.body.text.toUpperCase());
},
invoker.SignatureType.HTTP
invoker.SignatureType.HTTP,
'/'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied suggestion of testFunction.

test/invoker.ts Outdated
@@ -34,7 +34,8 @@ describe('request to HTTP function', () => {
(req: express.Request, res: express.Response) => {
res.send(req.body.text.toUpperCase());
},
invoker.SignatureType.HTTP
invoker.SignatureType.HTTP,
'/'
);
return supertest(server)
.post('/')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@grant grant merged commit 299f892 into master Sep 6, 2019
@grant grant deleted the grant_routing branch September 6, 2019 16:04
hdp617 added a commit that referenced this pull request Sep 14, 2019
@hdp617
Revert "fixes #33: Only listen to functionTarget. (#81)"
0b348e8 
This reverts commit 299f892.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hdp617 hdp617 hdp617 approved these changes

Assignees

@grant grant

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

functions called from browser execute twice
3 participants
@grant @hdp617 @googlebot