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

Changes for feedback from the Pub/Sub team #192

Merged
merged 14 commits into from
Jul 22, 2015

Conversation

blowmage
Copy link
Contributor

Make changes requested by the Pub/Sub team during their review. #58

  • Ensure Message#attributes returns a Hash (and not a Google API Client object)
  • Topic#pull should return immediately by default
  • Add Subscription#wait_for_messages alias for pull immediate: false
  • Add auto acknowledgement when pulling messages (Subscription#pull autoack: true)
  • Add Subscription#listen code
  • Document listen feature in main pub/sub docs
  • Add docs for subscribing to topic in a different project
  • Add project syntax (pubsub.topic "name", project: "project-id")
  • Add Topic#policy and Topic#policy= code/docs
  • Add Subscription#policy and Subscription#policy= code/docs

Calling a message's attributes should give you a hash, so you
can ask for the keys or iterate over the values.
The code was returning the Google API Client object, which
wraps the hash but does not have all the hash methods.
Convert the object to a hash using to_hash.

Ruby 1.9.3 does not have Hash#to_hash, so guard against that.
The documentation made it seem the default behavior was to not return
immediately. Update documentation to match the current behavior.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 17, 2015
@blowmage blowmage force-pushed the pubsub-revise branch 2 times, most recently from a5b88ad to 7726c37 Compare July 17, 2015 22:17
It was possible that waiting for a pull request to finish would
lead to an HTTP timeout error. Catch the errors and return an
empty list instead.

Forced to disable rubocop because none of the lines in pull
are at a different level of abstraction. No obvious choice
to extract methods. It would be more confusing to break
the method up.
This is analogous to calling Subscription#pull immediate: true.
Added for syntactical sugar.
Update documentation to use wait_for_messages.
The variable was copy/pasted and raises a warning. Remove it.
This option will acknowledge messages as they are pulled.
This is for convenience at the cost of possibly losing a
message that is acknowledged before it can be processed.

Beware...
Long polling messages, runs in an infinite loop.
@blowmage
Copy link
Contributor Author

/cc @tmatsuo @remi @jgeewax This includes the auto acknowledgement feature when pulling messages, as well as the listen feature. Thoughts?

Not every error returned has status, code, and errors in the response.
Guard against this by raising Gcloud::Pubsub::Error with response attr.
We want to avoid unhandled errors while raising a typed error. :)
The Google Cloud docs state that subscriptions can be created without
a name, and that a name will be generated in such cases. However, we
cannot create subscriptions without a name using Google API Client.
Remove the option to subscribe without a name until this is supported.

[refs googleapis#193]
We want to allow Pub/Sub topic objects to reference topics in
a different project. We allow this by passing in the full path,
but we also want to allow the following syntax:

    pubsub.topic topic-name, project: another-project-id

We implemented this optional parameter when creating full paths
for project, topic, and subscription, although they are not
documented. They are there when/if we need it.
@blowmage blowmage assigned blowmage and quartzmo and unassigned blowmage and quartzmo Jul 20, 2015
Expose the getIamPolicy and setIamPolicy endpoints.
There is no official documentation for these endpoints yet,
so this implementation is intentionally vague.
But this is important functionality and needs to be here.
@blowmage
Copy link
Contributor Author

@tmatsuo and @remi, thoughts?

@blowmage
Copy link
Contributor Author

@tmatsuo / @remi ping

@tmatsuo
Copy link
Contributor

tmatsuo commented Jul 21, 2015

LGTM for docs

@blowmage
Copy link
Contributor Author

@remi Thoughts? We'd like to get this released soon. :)

@blowmage
Copy link
Contributor Author

We will consider this is good to go and accept it so we can cut a new release.

quartzmo added a commit that referenced this pull request Jul 22, 2015
Additional Pub/Sub changes

These changes are based on feedback from the Pub/Sub team.

[closes #58]
@quartzmo quartzmo merged commit 8376c8c into googleapis:master Jul 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants