-
Notifications
You must be signed in to change notification settings - Fork 263
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
Pluggable hosting #108
Pluggable hosting #108
Conversation
Nice work - looking good overall. I'll try to take a closer look soon. |
Thanks. It seems that the build is broken for node 0.8, but I'm afraid I don't understand the failure. |
don't worry about node v0.8 |
Hello. No pressure, but did you have time to look into it ? |
not yet, but on my radar! |
} | ||
opts.host = add_trailing_slash(eval_template(package_json.binary.host,opts)); | ||
opts.host = opts.host ? add_trailing_slash(eval_template(package_json.binary.host,opts)) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why fallback to null here (and below)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because host is no longer a required option and add_trailing_slash fails if called on null or undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, makes sense. Can you share what an alternative hosting
might look like so I can ponder whether the host
related params might be moved around so things are more generic? I worry about having null
params making this brittle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see here for an example: https://github.com/albanm/node-addon-example/blob/master/package.json
I didn't move host around in order not to break compatibility, but following the logic of this PR it should probably be in a 'hosting' object in the 'binary' options. And the add_trailing_slash or other processing should be done in the specific hosting implementation.
Can you please pull in the latest changes from master? Tests are now refactored and disabled for node v0.8.x so they should be passing. Will be good to confirm everything still works with your changes. Thanks! |
Here you go! |
Is there any intention to continue with this PR? I was about to tackle something similar. |
@indieisaconcept - I would prefer there be tools outside of node-pre-gyp to automate the upload of binaries to custom hosts. This has the advantage of keeping node-pre-gyp simple, but not holding anyone back who thinks of yet another place they would like to store binaries. So, my proposal for how to do this is to write a script that hooks into node-pre-gyp either via command line calls (or programmatically). So you if want to push easily to bintray you could write a |
@springmeyer thanks. I was considering revising node-pre-gyp to support the notion of publish adapters essentially removing all publish logic other than just simple orchastration. I was then going to write separate adapters for s3, releases & bintray. This PR looked like it was going in this direction hence the comment. I think there is value in pulling s3 out of core though and I like the idea of a standardised way of describing a publish adapter. ( See travis/travis-dpl ) I would like to see adapters having an interface as such.
On Wed, Mar 25, 2015 at 3:47 AM, Dane Springmeyer
|
Hello. One thing this PR does not is: "holding anyone back who thinks of yet another place they would like to store binaries". Quite the contrary actually. It makes node-pre-gyp pluggable, if the upload logic is already available in a module 'node-bintray' for example, just create a 'node-pre-gyp-hosting-bintray' module that bridges the gap. If the upload logic is purely specific to a user, just write a 'node-pre-gyp-hosting-custom' plugin with a script loading mechanism and which would accept some parameters similar to this :
The custom user script would have to implement the 'publish', 'unpublish' and 'download' methods just as other plugins. Of course you don't have to create a separate 'node-pre-gyp-hosting-custom' project, just bring it with the main project as a officially supported hosting implementation in 'lib/util/hosting-custom.js' alongside with 'lib/util/hosting-s3.js'. Hope this helps ! |
+1 for adding this directly in node-pre-gyp. It seems like it's a basic necessity. I almost didn't notice that the work was done and therefore almost wrote something similar. But more importantly the reason it should be included is because I consider storing binaries in github releases more of the default then S3. If there was one extra api that should be directly integrated into node-pre-gyp this is it. |
I agree with @bchr02, publish to GitHub is not recommended but it's free, while at the same time S3 is a paid option. As the README says GitHub is just enough for smaill projects, so I think this pull-request should be included and GitHub publishing set as default. |
@piranna So you know, I ended up creating a module that complements node-pre-gyp called node-pre-gyp-github which allows publishing to GitHub releases. Instead of @springmeyer if your not going to add this support, maybe you can update your README.md to mention my module? Thanks. |
Closing since |
cf #106
Companion project: https://github.com/albanm/node-pre-gyp-hosting-github
The two together are tested in my fork of your node-addon-example project https://github.com/albanm/node-addon-example
I couldn't test that I didn't break s3 support as I don't have an account. I guess travis-ci will tell me soon enough if I did ok.
I didn't externalize s3 hosting in order not to break compatibility, but if you chose to do so it will be easy.
Of course I am open to all feedback.