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

Adding the ability to set Subscription path #919

Conversation

philibuster
Copy link

A user should be able to set the path of the subscription, so it isn't always set to the topics project.

This allow users to subscribe to topics in other projects (if they have edit rights to said project).

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Jun 8, 2015
@philibuster philibuster force-pushed the add-setter-to-subscription-path branch from 076b491 to 3567321 Compare June 8, 2015 23:26
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jun 8, 2015
@philibuster philibuster force-pushed the add-setter-to-subscription-path branch 3 times, most recently from 8aecf6a to cd8460a Compare June 8, 2015 23:40
@philibuster philibuster force-pushed the add-setter-to-subscription-path branch from cd8460a to 3121e40 Compare June 8, 2015 23:50
@dhermes
Copy link
Contributor

dhermes commented Jun 9, 2015

@philibuster Can you give some sample code using this or some description of why you'd want to be able to change the project on the fly.

This change is a little too ad-hoc for the object model, but that doesn't mean we can't take the spirit of the change and turn it into something useful.


A subscription has 4 properties

name
topic
ack_deadline
push_endpoint

and you could accomplish essentially what you want with

topic1 = Topic(name, project=project1)
topic2 = Topic(name, project=project2)

subscription1 = Subscription(sub_name, topic1, ack_deadline=AD, push_endpoint=PE)
subscription2 = Subscription(sub_name, topic2, ack_deadline=AD, push_endpoint=PE)

but it could be more convenient to have something which preserved all properties except for the topic via:

subscription2 = Subscription.copy_from(subscription1, topic=topic2)

Even more, you want the same topic name, but just a different project, so this still isn't quite the same.

@philibuster
Copy link
Author

Thank you for the response. Sorry I made a mistake in my code, what I originally wrote would have no value.

Example:
There are two projects:

  1. project1 with one topic "topic1"
  2. project2 with no topics (it doesnt matter)

As project2 I want to make a subscription to the other projects topic.
If I made a subscription it will try use the first project. projects/project1/subscriptions/sub with project/project1/topics/topic1
But I want projects/project2/subscriptions/sub with project/project1/topics/topic1

What I am proposing is using any project in the subscription path. It still will only work if they are allowed access to create the subscription anyway.

@philibuster philibuster force-pushed the add-setter-to-subscription-path branch from 659e5e7 to 3121e40 Compare June 10, 2015 19:49
@philibuster philibuster force-pushed the add-setter-to-subscription-path branch from 18c04af to 70d9787 Compare June 10, 2015 22:41
@dhermes
Copy link
Contributor

dhermes commented Jun 17, 2015

I see. So my proposal would need to be tweaked to something like

>>> topic = Topic(name, project=project1)
>>> subscription = Subscription(sub_name, topic, ack_deadline=AD, push_endpoint=PE)
>>> # New code
>>> subscription2 = Subscription.copy_from(subscription1, project=project2)
>>> subscription.topic.name == subscription2.topic.name
True
>>> subscription.topic is subscription2.topic
False
>>> subscription.topic.project == subscription2.topic.project
False

PS @philibuster Sorry for getting behind on this.

@philibuster
Copy link
Author

@dhermes No problem :)

Not exactly, the topic should not be changed, just the subscription path

>>> subscription.topic == subscription2.topic
True
>>> subscription.path == subscription2.path
False

@dhermes
Copy link
Contributor

dhermes commented Jun 18, 2015

@philibuster That breaks the model, which is why I was offering the alternative.

As of right now (June 18), the hierarchy is

client -> topic -> subscription

and a client has a fixed project, which both the topic and subscription can reference and use.

@dhermes
Copy link
Contributor

dhermes commented Jun 24, 2015

@philibuster WDYT of the proposed given my comment?

I want to keep this moving (either to close it out or to get to something we can agree on implementing).

@dhermes
Copy link
Contributor

dhermes commented Jul 2, 2015

@philibuster I'd love to take this up, let's re-open if you get some bandwidth to keep the discussion moving forward.

@dhermes dhermes closed this Jul 2, 2015
@philibuster
Copy link
Author

@dhermes Sorry about the delay.

Yeah all for a copy_from method, maybe similar to modify_push_configuration. I think it'd be a useful method (at least to me and those that work with me), as it'd allow subscribing to other projects topics.

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.

4 participants