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

Integrate rubygems.org and gittip #505

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
3 participants
@tehgeekmeister
Contributor

tehgeekmeister commented Jan 10, 2013

Setting up a pull request early and pushing early and often so as to get feedback on any desired changes early. Will update when I actually want this merged.

Also, it's important to note that this is only gem level integration. Maintainer specific integration comes next.

Current status:

  • Enough functionality to look like it works.
  • Need an icon for enabling gittip if we keep the current UI.
  • Need to do callbacks over to gittip on update so that this all actually works.
@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Jan 10, 2013

Contributor

#500 is the relevant issue.

Contributor

tehgeekmeister commented Jan 10, 2013

#500 is the relevant issue.

@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Jan 10, 2013

Contributor

I see the build is still failing here. I'll make sure to address that as well, before I get to considering this done.

Contributor

tehgeekmeister commented Jan 10, 2013

I see the build is still failing here. I'll make sure to address that as well, before I get to considering this done.

@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Jan 14, 2013

Contributor

Just rebased this so it'd be merge-free. Do you prefer it squashed, too? Would've written tests, but I'm not quite sure what to test in this case.

Contributor

tehgeekmeister commented Jan 14, 2013

Just rebased this so it'd be merge-free. Do you prefer it squashed, too? Would've written tests, but I'm not quite sure what to test in this case.

@indirect

This comment has been minimized.

Show comment
Hide comment
@indirect

indirect Jan 14, 2013

Member

First step seems to be getting the build green? :)

Member

indirect commented Jan 14, 2013

First step seems to be getting the build green? :)

@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Jan 14, 2013

Contributor

Right!

Contributor

tehgeekmeister commented Jan 14, 2013

Right!

@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Jan 14, 2013

Contributor
1Using worker: ruby3.worker.travis-ci.org:ruby-5
2
3
4
5
6I'm sorry but an error occured within Travis while running your build.
7
8We are continuosly working on test run stability, please email support@travis-ci.org if this error persists.
9
10Below is the stacktrace of the error:
11
12Error: #<Travis::Worker::VirtualMachine::VmFatalError: The VM had trouble shutting down and has now been told off (forcefully killed), your build will be requeued shortly.>
13  /home/travis/travis-worker/lib/travis/worker/virtual_machine/virtual_box.rb:203:in `close_sandbox'
14  /home/travis/travis-worker/lib/travis/worker/virtual_machine/virtual_box.rb:200:in `close_sandbox'
15  /home/travis/travis-worker/lib/travis/worker/virtual_machine/virtual_box.rb:129:in `sandboxed'
16  /home/travis/travis-worker/vendor/bundle/jruby/1.9/bundler/gems/travis-build-486121f54bf2/lib/travis/build/remote.rb:25:in `perform'
17  /home/travis/travis-worker/vendor/bundle/jruby/1.9/bundler/gems/travis-build-486121f54bf2/lib/travis/build.rb:52:in `run'
18  org/jruby/RubyBasicObject.java:1659:in `__send__'
19  org/jruby/RubyKernel.java:2086:in `send'
20  /home/travis/travis-worker/vendor/bundle/jruby/1.9/bundler/gems/travis-support-e3e09d89fc8d/lib/travis/support/logging.rb:26:in `run_with_log'
21  /home/travis/travis-worker/lib/travis/worker.rb:246:in `hard_timeout'
22  /home/travis/travis-worker/lib/hard_timeout.rb:11:in `timeout'

Looks like it might be unrelated to my changes, but whether it is or not, I'm unsure how to troubleshoot. Will keep poking around.

Contributor

tehgeekmeister commented Jan 14, 2013

1Using worker: ruby3.worker.travis-ci.org:ruby-5
2
3
4
5
6I'm sorry but an error occured within Travis while running your build.
7
8We are continuosly working on test run stability, please email support@travis-ci.org if this error persists.
9
10Below is the stacktrace of the error:
11
12Error: #<Travis::Worker::VirtualMachine::VmFatalError: The VM had trouble shutting down and has now been told off (forcefully killed), your build will be requeued shortly.>
13  /home/travis/travis-worker/lib/travis/worker/virtual_machine/virtual_box.rb:203:in `close_sandbox'
14  /home/travis/travis-worker/lib/travis/worker/virtual_machine/virtual_box.rb:200:in `close_sandbox'
15  /home/travis/travis-worker/lib/travis/worker/virtual_machine/virtual_box.rb:129:in `sandboxed'
16  /home/travis/travis-worker/vendor/bundle/jruby/1.9/bundler/gems/travis-build-486121f54bf2/lib/travis/build/remote.rb:25:in `perform'
17  /home/travis/travis-worker/vendor/bundle/jruby/1.9/bundler/gems/travis-build-486121f54bf2/lib/travis/build.rb:52:in `run'
18  org/jruby/RubyBasicObject.java:1659:in `__send__'
19  org/jruby/RubyKernel.java:2086:in `send'
20  /home/travis/travis-worker/vendor/bundle/jruby/1.9/bundler/gems/travis-support-e3e09d89fc8d/lib/travis/support/logging.rb:26:in `run_with_log'
21  /home/travis/travis-worker/lib/travis/worker.rb:246:in `hard_timeout'
22  /home/travis/travis-worker/lib/hard_timeout.rb:11:in `timeout'

Looks like it might be unrelated to my changes, but whether it is or not, I'm unsure how to troubleshoot. Will keep poking around.

@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Jan 14, 2013

Contributor
1Using worker: ruby1.worker.travis-ci.org:ruby-6
2
3$ cd ~/builds
4$ git clone --depth=100 --quiet git://github.com/rubygems/rubygems.org.git rubygems/rubygems.org
5
6
7Executing your  (rm -f ~/.ssh/source_rsa) took longer than 3 minutes and was terminated.
8
9For more information about the test timeouts please checkout the section Build Timeouts at http://about.travis-ci.org/docs/user/build-configuration/.
10
11
12
13Done. Build script exited with: 1

Looks probably unrelated, but again, I can't really tell, and I'm not familiar with travis.

Contributor

tehgeekmeister commented Jan 14, 2013

1Using worker: ruby1.worker.travis-ci.org:ruby-6
2
3$ cd ~/builds
4$ git clone --depth=100 --quiet git://github.com/rubygems/rubygems.org.git rubygems/rubygems.org
5
6
7Executing your  (rm -f ~/.ssh/source_rsa) took longer than 3 minutes and was terminated.
8
9For more information about the test timeouts please checkout the section Build Timeouts at http://about.travis-ci.org/docs/user/build-configuration/.
10
11
12
13Done. Build script exited with: 1

Looks probably unrelated, but again, I can't really tell, and I'm not familiar with travis.

@indirect

This comment has been minimized.

Show comment
Hide comment
@indirect

indirect Jan 14, 2013

Member

Blah. Those both look like pure Travis failures. I'll try to restart the build.

Member

indirect commented Jan 14, 2013

Blah. Those both look like pure Travis failures. I'll try to restart the build.

@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Jan 14, 2013

Contributor

Thanks.

Contributor

tehgeekmeister commented Jan 14, 2013

Thanks.

@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Jan 14, 2013

Contributor

Alright, well I'm just going to start finalizing things and running tests locally until we get this resolved, and then move on to the next steps as discussed in #500.

Contributor

tehgeekmeister commented Jan 14, 2013

Alright, well I'm just going to start finalizing things and running tests locally until we get this resolved, and then move on to the next steps as discussed in #500.

@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Jan 14, 2013

Contributor

Tests pass locally, squashed, rebased, and ready to merge (once it meets your and travis' approval, @indirect).

Contributor

tehgeekmeister commented Jan 14, 2013

Tests pass locally, squashed, rebased, and ready to merge (once it meets your and travis' approval, @indirect).

@indirect

This comment has been minimized.

Show comment
Hide comment
@indirect

indirect Jan 20, 2013

Member

Travis says this is good! Thanks for writing this. :) I may be wrong, but I don't see the gittip.png image anywhere in the squashed commit — am I just missing it, or does it still need to be added?

Member

indirect commented Jan 20, 2013

Travis says this is good! Thanks for writing this. :) I may be wrong, but I don't see the gittip.png image anywhere in the squashed commit — am I just missing it, or does it still need to be added?

@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Jan 20, 2013

Contributor

That! I put it on the other PR, I think, but not this one. Fixing!

Contributor

tehgeekmeister commented Jan 20, 2013

That! I put it on the other PR, I think, but not this one. Fixing!

@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Jan 20, 2013

Contributor

Committed separately (wanted to make sure I got it right, my git is wonky on this clone right now). Squashing presently!

Contributor

tehgeekmeister commented Jan 20, 2013

Committed separately (wanted to make sure I got it right, my git is wonky on this clone right now). Squashing presently!

@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Mar 13, 2013

Contributor

Alright, this is (at least kinda) alive again! It should be sufficient now for integration to be completed, but there are admittedly at least two deficiencies in the PR as it stands:

  1. API changes should be documented.
  2. There should probably be tests on the new API endpoint, but I honestly don't know what to put there. 😕
  3. That other thing I didn't find but one of you will...

On the other hand, @whit537 is ready to wrap up all the gratipay/gratipay.com#233 stuff on this ASAP so the integration can be announced at PYCON this weekend, so I'm willing to really prioritize this as much as I can, if I can just get feedback telling me what's necessary to get this up to snuff quickly.

Contributor

tehgeekmeister commented Mar 13, 2013

Alright, this is (at least kinda) alive again! It should be sufficient now for integration to be completed, but there are admittedly at least two deficiencies in the PR as it stands:

  1. API changes should be documented.
  2. There should probably be tests on the new API endpoint, but I honestly don't know what to put there. 😕
  3. That other thing I didn't find but one of you will...

On the other hand, @whit537 is ready to wrap up all the gratipay/gratipay.com#233 stuff on this ASAP so the integration can be announced at PYCON this weekend, so I'm willing to really prioritize this as much as I can, if I can just get feedback telling me what's necessary to get this up to snuff quickly.

@indirect

This comment has been minimized.

Show comment
Hide comment
@indirect

indirect Mar 13, 2013

Member

Is the plan to announce integration with rubygems, or integration with language package repos at Pycon?

Member

indirect commented Mar 13, 2013

Is the plan to announce integration with rubygems, or integration with language package repos at Pycon?

@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Mar 13, 2013

Contributor

Just rubygems, AFAIK. No other language community has stepped up to the challenge, so maybe we can shame python into catching up. :trollface:

Contributor

tehgeekmeister commented Mar 13, 2013

Just rubygems, AFAIK. No other language community has stepped up to the challenge, so maybe we can shame python into catching up. :trollface:

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Mar 13, 2013

Contributor

The idea of announcing at PyCon was mostly a joke. :) Though you're right that having this would certainly get the ball rolling on a conversation with the PyPI maintainers.

PyCon doesn't need to be a deadline, in other words. Let's knock this out in good form and announce it as we're able.

Contributor

chadwhitacre commented Mar 13, 2013

The idea of announcing at PyCon was mostly a joke. :) Though you're right that having this would certainly get the ball rolling on a conversation with the PyPI maintainers.

PyCon doesn't need to be a deadline, in other words. Let's knock this out in good form and announce it as we're able.

@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Mar 13, 2013

Contributor

That makes more sense. I was too far into analytical mode to notice the joke when I commented, actually. Anyway, I'd love to get this wrapped up soonish. I'm happy to do whatever will help in expediting that.

Also, looks like the build failures are unrelated again. 😢

Contributor

tehgeekmeister commented Mar 13, 2013

That makes more sense. I was too far into analytical mode to notice the joke when I commented, actually. Anyway, I'd love to get this wrapped up soonish. I'm happy to do whatever will help in expediting that.

Also, looks like the build failures are unrelated again. 😢

@indirect

View changes

Show outdated Hide outdated app/controllers/api/v1/profiles_controller.rb
@@ -0,0 +1,7 @@
class Api::V1::ProfilesController < Api::BaseController
respond_to :yaml, :xml, :json, :only => [:show]

This comment has been minimized.

@indirect

indirect Mar 21, 2013

Member

Pretty sure that YAML and XML are out as options after the security debacles of late?

@indirect

indirect Mar 21, 2013

Member

Pretty sure that YAML and XML are out as options after the security debacles of late?

This comment has been minimized.

@tehgeekmeister

tehgeekmeister Mar 21, 2013

Contributor

I'm on an old-ish version of the code still, and just followed the convention I found in other controllers as of then. Good catch! Will fix it now.

@tehgeekmeister

tehgeekmeister Mar 21, 2013

Contributor

I'm on an old-ish version of the code still, and just followed the convention I found in other controllers as of then. Good catch! Will fix it now.

@indirect

View changes

Show outdated Hide outdated app/models/user.rb
@@ -55,7 +55,10 @@ def all_hooks
end
def payload
{"email" => email}
attrs = {"email" => email}
attrs.merge!({"gittip_username" => gittip_username}) if gittip_username

This comment has been minimized.

@indirect

indirect Mar 21, 2013

Member

Curly braces aren't needed inside parens, but I'd probably just implement this as attrs["gittip_username"] = gittip_username if gittip_username.

@indirect

indirect Mar 21, 2013

Member

Curly braces aren't needed inside parens, but I'd probably just implement this as attrs["gittip_username"] = gittip_username if gittip_username.

This comment has been minimized.

@tehgeekmeister

tehgeekmeister Mar 21, 2013

Contributor

I agree. Changing it.

@tehgeekmeister

tehgeekmeister Mar 21, 2013

Contributor

I agree. Changing it.

This comment has been minimized.

@tehgeekmeister

tehgeekmeister Mar 21, 2013

Contributor

(I'm actually pretty baffled as to why I did that in the first place, actually. Oh well.)

@tehgeekmeister

tehgeekmeister Mar 21, 2013

Contributor

(I'm actually pretty baffled as to why I did that in the first place, actually. Oh well.)

@indirect

This comment has been minimized.

Show comment
Hide comment
@indirect

indirect Mar 21, 2013

Member

This looks great to me. I'm trying to get the tests running locally, and then I guess I'll try to kick travis until it gives in and runs the tests again (but I don't think I can do that for pull requests).

Member

indirect commented Mar 21, 2013

This looks great to me. I'm trying to get the tests running locally, and then I guess I'll try to kick travis until it gives in and runs the tests again (but I don't think I can do that for pull requests).

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Mar 26, 2013

Contributor

Okay, I just reviewed f39866dbe517c1b71f65c5eeacb3a8927c20cc7f.

  • I believe we need to fix up the links to Gittip.
  • The only json reference I see is in f7b4f6ab64a45a77d754377183ed4d734818c187. That gives us what we need?
Contributor

chadwhitacre commented Mar 26, 2013

Okay, I just reviewed f39866dbe517c1b71f65c5eeacb3a8927c20cc7f.

  • I believe we need to fix up the links to Gittip.
  • The only json reference I see is in f7b4f6ab64a45a77d754377183ed4d734818c187. That gives us what we need?
@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Apr 24, 2013

Contributor

Bump.

Contributor

chadwhitacre commented Apr 24, 2013

Bump.

@indirect

This comment has been minimized.

Show comment
Hide comment
@indirect

indirect May 13, 2013

Member

Happy to merge this once it's ready to go. :)

Member

indirect commented May 13, 2013

Happy to merge this once it's ready to go. :)

tehgeekmeister added some commits Jan 10, 2013

profiles can link to gittip. api changes for gittip integration.
Users can add a gittip username to their profile, which will cause
their profile page and the gem pages for any gems they own to link
to relevant pages on gittip.

As well, some API changes were needed so gittip could integrate
effectively.  These include:
  1) gems/:gem_name/owners.:format now includes the gittip_username
     when the relevant profile has opted in.
  2) profiles/:username.:format now exists, and returns profile info.
@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister May 16, 2013

Contributor

Alright, I did the link-tweaks requested. The build failure does seem to have been legit last time, but unfortunately I can't replicate it on my machine, even with destroying the databases and rebuilding them before running tests. I've got it rebased onto master, too.

Contributor

tehgeekmeister commented May 16, 2013

Alright, I did the link-tweaks requested. The build failure does seem to have been legit last time, but unfortunately I can't replicate it on my machine, even with destroying the databases and rebuilding them before running tests. I've got it rebased onto master, too.

indirect added a commit that referenced this pull request Aug 2, 2013

Merge pull request #505 from tehgeekmeister/master
Integrate rubygems.org and gittip
@tehgeekmeister

This comment has been minimized.

Show comment
Hide comment
@tehgeekmeister

tehgeekmeister Oct 15, 2013

Contributor

Closing since this is merged. Glad this finally made it out!

Contributor

tehgeekmeister commented Oct 15, 2013

Closing since this is merged. Glad this finally made it out!

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Oct 15, 2013

Contributor

👯

Contributor

chadwhitacre commented Oct 15, 2013

👯

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Dec 1, 2014

Contributor

Is this closed or merged? I'm confused.

Contributor

chadwhitacre commented Dec 1, 2014

Is this closed or merged? I'm confused.

indirect added a commit that referenced this pull request Jan 29, 2015

Merge pull request #505 from tehgeekmeister/master
Integrate rubygems.org and gittip
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment