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

Speling: pass 'ack_deadline' as 'ackDeadlineSeconds' everywhere. #1182

Merged
merged 2 commits into from
Oct 15, 2015
Merged

Speling: pass 'ack_deadline' as 'ackDeadlineSeconds' everywhere. #1182

merged 2 commits into from
Oct 15, 2015

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Oct 14, 2015

May fix #1169.

@tseaver tseaver added the api: pubsub Issues related to the Pub/Sub API. label Oct 14, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 14, 2015
@dhermes
Copy link
Contributor

dhermes commented Oct 14, 2015

Can you update the system tests to avoid this issue coming up again?

Also, can you link to a doc describing this parameter.

@tseaver
Copy link
Contributor Author

tseaver commented Oct 14, 2015

Can you update the system tests to avoid this issue coming up again?

I guess I could: we don't have anything like exhaustive coverage in the system tests, though.

Also, can you link to a doc describing this parameter.

https://cloud.google.com/pubsub/reference/rest/v1/projects.subscriptions#Subscription.FIELDS.ack_deadline_seconds

(note the irony that the ID for the table cell is closer to our spelling than to the one used in the actual API).

@dhermes
Copy link
Contributor

dhermes commented Oct 14, 2015

I think the reason our system tests were ever named regression tests was for reasons like the very bug that caused this.

Let's add one to make sure it doesn't come back.


How could we be more comprehensive? Should we?

@tseaver
Copy link
Contributor Author

tseaver commented Oct 15, 2015

Let's add one to make sure it doesn't come back.

543caaf adds that test.

How could we be more comprehensive? Should we?

Dunno: for instance, the other optional parameter to Subscription.create() is push_endpint, but we won't be able to exercise it without having an actual, approved URL.

@dhermes
Copy link
Contributor

dhermes commented Oct 15, 2015

Great! LGTM

(We have similar issues with Bigtable)

tseaver added a commit that referenced this pull request Oct 15, 2015
…_json_payload

Speling: pass 'ack_deadline' as 'ackDeadlineSeconds' everywhere.
@tseaver tseaver merged commit eb3d390 into googleapis:master Oct 15, 2015
@tseaver tseaver deleted the 1169-pubsub-fix_ack_deadline_in_json_payload branch October 15, 2015 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PubSub subscription with ack_deadline set causes HTTP 400
3 participants