Skip to content

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

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
Merged
tseaver merged 2 commits into from
Oct 15, 2015
Merged

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

tseaver 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
@tseaver
Merge pull request #1182 from tseaver/1169-pubsub-fix_ack_deadline_in…
eb3d390 
…_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
@dhermes dhermes mentioned this pull request Jan 25, 2016
Merged
parthea pushed a commit that referenced this pull request Nov 26, 2025
@BigTailWolf
fix: updated the lower bound of interactive timeout and fix the kwarg… (
c63652c 
#1182)

* fix: udpated the lower bound of interactive timeout and fix the kwargs invalid syntax

* update token

* update token

* prohibit the access from constructor and only allow the injection and test

* fix lint

* adding interactive timeout validation test

* update token
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

api: pubsub Issues related to the Pub/Sub API. 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.

PubSub subscription with ack_deadline set causes HTTP 400

3 participants

@tseaver @dhermes @googlebot