search

boxen / puppet-nodejs

MIT License
15 stars 47 forks source link

rewrite based on boxen/puppet-ruby #48

Closed blackjid closed 9 years ago

blackjid commented 9 years ago

This is done to get all the benefits from the latests puppet-ruby rewrite.

There are some breaking changes, but I this it worth it

blackjid commented 9 years ago

I someone wants to merge this PR I can squash al those commits before the merge.

rafaelfranca commented 9 years ago

I'll take a closer look but I'm positive for the idea. Also I'd love to move nodenv to boxen organization.

On Wed, Apr 15, 2015, 19:28 Juan Ignacio Donoso notifications@github.com wrote:

I someone wants to merge this PR I can squash al those commits before the merge.

— Reply to this email directly or view it on GitHub https://github.com/boxen/puppet-nodejs/pull/48#issuecomment-93586985.

blackjid commented 9 years ago

@rafaelfranca Have you had the time to take a look at the PR? please if need something from me, just shoot! :)

seanknox commented 9 years ago

@OiNutter can you explain the difference between your nodenv and https://github.com/wfarr/nodenv ?

blackjid commented 9 years ago

For what I can see, OiNutter's nodenv is more full featured. But @wfarr doesn't thought so in this issue https://github.com/wfarr/nodenv/issues/11.

I can see this functionality on OiNutter's nodenv that is not on wfarr's nodenv:

It's basically a copy of sstephenson's rbenv with all it's functionality Maybe the only pain coming from wfarr's nodenv, is that node versions are defined without the leading v (0.10.36 instead of v0.10.36) which I like better.

it would be awesome to know from @OiNutter himself

blackjid commented 9 years ago

@seanknox @rafaelfranca sorry, I just want to know your thoughts, do you think this is going to happen???

blackjid commented 9 years ago

I've just rebased against master again :)

OiNutter commented 9 years ago

@blackjid Yeah that's pretty much the case, the only question that's still outstanding is I'm not sure how well mine handles precompiled binaries. It was a question that came up when we were discussing which one should be included in homebrew by default.

blackjid commented 9 years ago

@rafaelfranca @seanknox Do you think that this is going to be merge ever??

blackjid commented 9 years ago

@rafaelfranca could you please add a github token to the repo in travis to prevent the API rate limit exceeded for issue? thanks!

MikeMcQuaid commented 9 years ago

Writes like this scare me a little. At the very least we need to get a green build. Do all the user-facing APIs remain identical?

blackjid commented 9 years ago

hey @mikemcquaid, the tests should be passing now. I thought the problem wasn't on my end, but I was wrong...

The rewrite is pretty is kind big, much the same as the puppet-ruby 8.0.0 rewrite. The user facing api, was changed a little.

If this is merged, clearly is a major version change... I've been using it for a while, and works way better for me.... I think it should for everyone

MikeMcQuaid commented 9 years ago

I think we should avoid those changes which aren't 100% necessary. An obvious example seems to be /opt/nodes; unless you're automatically migrating the nodes across here it doesn't seem to be a great idea. Basically everything here should be backwards compatible for this to be merged.

blackjid commented 9 years ago

I don't really get why would you migrate those nodes... New nodes defined in boxen manifest should get installed on /opt/nodes and symlinked to /opt/boxen/nodenv/versions. Only the packages installed by hand are going to be missing.

I also see the option that someone want to use another node version manager like https://github.com/moll/sh-chnode , that why I think the guys that rewrote puppet-ruby put the rubies in /opt/rubies in the first place and added the provider option.

MikeMcQuaid commented 9 years ago

Only the packages installed by hand are going to be missing.

Yep, and that's something that warrants migration.

blackjid commented 9 years ago

mmm, ok... should I add that migration?? or move /opt/nodes to /opt/boxen/nodenv/nodes?? what do you think???

BTW, why puppet-ruby didn't do that migration with the gems in 8.0.0 and was merge anyway??? a bad call from how accepted the PR??

MikeMcQuaid commented 9 years ago

mmm, ok... should I add that migration?? or move /opt/nodes to /opt/boxen/nodenv/nodes?? what do you think???

Yeh, please. I'd like it so that for Boxen administrators they can upgrade this and with no code changes have everything migrated and working.

BTW, why puppet-ruby didn't do that migration with the gems in 8.0.0 and was merge anyway??? a bad call from how accepted the PR??

Yeh, difference of opinion on best practices, probably.

blackjid commented 9 years ago

hey @mikemcquaid... I've been working on adding this migration... but I still think that maybe is not necessary

Only the packages installed by hand are going to be missing.

npm packages installed by hand are changing the state defined buy pupept/boxen. Is weird to just copy files from a location that supposed to have old nodes....

This is what I started doing https://github.com/platanus/puppet-nodejs/commit/3861361260f2bc252c20351210235b261eec1bde It works, at least in the initial tests. But now I have the problema of the differently named versions.

Maybe other approach to the migration would be to write a fact that reads the current installed node versions and npm packages for each version and ensure those resources with the new node module....

I have been waiting this to be merge for a long time, and I haven't been able to deploy this to my team. Today I just did that from our org fork, so please if you think it will be to much work or a problem to push this to everybody, don't hesitate on close the PR without merge. that would be sad, but its ok.

Any thoughts??

jemmyw commented 9 years ago

I think migrating is a bad idea and bound to be error prone. Bumping the version for a major change should be enough warning to those like myself maintaining boxen configs that it will change.

If individuals in my organisation have used nodenv to install other node versions and have not committed that configuration back to boxen then it's that individual's responsibility to maintain it.

2c because I'm eager to see this merged.

MikeMcQuaid commented 9 years ago

I'm OK with merging this if @blackjid is OK with being added to this repository as a contributor, watching the repository and working on fixing any issues that may result from it being merged. How does that sounds?

blackjid commented 9 years ago

sounds good for me...

MikeMcQuaid commented 9 years ago

@blackjid You now have commit access to this repo. Good luck :wink:

blackjid commented 9 years ago

Thanks.

Should I make a release? What do you think?

MikeMcQuaid commented 9 years ago

@blackjid Good idea!