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

Add BitBucket support #37

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

stephenmathieson
Copy link
Contributor

  • separate github stuff from main api
  • add primitive "providers" (github, bitbucket)
  • simple test case proving bitbucket support works

this is really just to get a code review. ideally, we'd move the provider modules into their own respective repos, but i didn't want to start creating duo-* without some feedback.

Stephen Mathieson added 3 commits August 6, 2014 13:14
@yields
Copy link
Contributor

yields commented Aug 6, 2014

nice!

@@ -23,6 +22,9 @@ var zlib = require('zlib');
var fs = require('co-fs');
var url = require('url');
var join = path.join;
var github = require('./github');

var auth = github.auth;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I wonder if auth should be a function, just like resolve and tarball. what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that makes sense for consistency sake.

@matthewmueller
Copy link
Contributor

thanks @stephenmathieson!

I think this is getting pretty close. I'm not sure I like how resolve is the only thing that requires a callback function. auth and tarball are basically read-only values (though one is a function one is not) while resolve does a bunch of provider-specific work. I think we could organize this into something a bit cleaner.

Maybe this is just the way the problem ends up unfolding, but I'm not quite sold yet on this implementation as it stands.

@matthewmueller
Copy link
Contributor

So basically right now the API looks like:

provider.auth
provider.tarball(repo, ref)
provider.resolve(repo, ref, fn)

I think if this is only going to work for duo, we might be able to get away with:

provider.auth()
provider.tarball(repo, ref)
*provider.resolve(repo, ref)

but even then, i don't like how one function is doing pretty much all the work... could we just have bb-resolve and gh-resolve or do we need the other stuff?

@stephenmathieson
Copy link
Contributor Author

could we just have bb-resolve and gh-resolve or do we need the other stuff?

yes and no. the resolve stuff is the majority of the work, but the tarball endpoints will differ from provider-to-provider:

  • github: https://<auth>github.com/<owner>/<repo>/archive/<ref>.tar.gz
  • bitbucket: https://<auth>bitbucket.org/<owner>/<repo>/get/<ref>.tar.gz
  • npm: http://registry.npmjs.org/<repo>/-/<repo>-<ref>.tgz

*provider.resolve(repo, ref)

yeah, i agree. i'm already calling thunkify(provider.resolve), but thats pretty hacky. ill refactor in a bit.

@stephenmathieson
Copy link
Contributor Author

@matthewmueller pushed some new commits and:

  • .resolve() is now a generator
  • .auth() is now a function

@dominicbarnes
Copy link
Contributor

Is there any reason this isn't being merged? (don't want it to grow stale, it is a nice step forward)

@stephenmathieson
Copy link
Contributor Author

@dominicbarnes iirc, stability/ready for being open sourced > bitbucket

@matthewmueller
Copy link
Contributor

Yah, I want to merge this, but I'm not sure now is the right time :-/

@matthewmueller
Copy link
Contributor

After Duo is out and we're more stable (travis, coveralls, etc.) I'll feel more comfortable introducing big new features.

@stryju
Copy link

stryju commented Apr 7, 2015

any progress on that? :)

@matthewmueller
Copy link
Contributor

I should have merged this at the time, just came right as we were gearing up to release. I'd be down to merge this now if we can update it.

@stephenmathieson
Copy link
Contributor Author

@matthewmueller i've got a private fork with bitbucket support, but it's severely out of date with everything. i'll try to get something together soonish though.

@stephenmathieson
Copy link
Contributor Author

is the proposed api here still acceptable btw?

/cc @dominicbarnes @matthewmueller @yields

@dominicbarnes
Copy link
Contributor

+1 from me, would really like to see a big step forward like this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants