search

wolfeidau / rake-bamboo-plugin

Plugin for bamboo which enables building of ruby projects using rake
Other
34 stars 16 forks source link

Feature/bundle cli options with backwards compatibility #51

Closed rosskevin closed 11 years ago

rosskevin commented 11 years ago

This is a compatible change.

The old Bundler task was repackaged and renamed as Bundler Install but retains the same @key so all existing plans continue to function.

This feature branch:

NOTE: The integration test is commented out. Otherwise, new tests have been added and all tests pass.

Fixes:

32

wolfeidau commented 11 years ago

I am happy to merge most of this as there are a lot of good improvements there.

Only thing I can recommend is do a few pull requests via separate branches next time, that way I can merge each chunk.

The one thing I am not sold on at the moment is the idea of renaming the bundle task / adding a new bundle task.

So my thoughts are the name of the old bundler task was silly it should have been bundle install that i agree with.

But the new bundle task is just run this thing with bundler.. What if i want to run the thing without bundler? Isn't it just run this ruby script, optionally with bundler?

rosskevin commented 11 years ago

All of these chunks had one goal, I just commit a lot to make changes self-documenting. Breaking this up into multiple pulls didn't make sense because I would need to do many pull requests and continue working off the assumption that the prior pull would get accepted.

The new Bundler CLI task is always a bundle command, sometimes with exec sometimes without, always with arguments. So while it does appear true that ruby bundle is called, it is not a free form ruby script. It is always bundle with arguments.

If your concern is that this is just a ruby script, than you could say the same for bundle install and rake too, but I would disagree. I think the separate tasks provide value and structure to the Bamboo UI, reducing potential for common errors. As the pull has it, there are:

While we could combine the bundler cli and install into one, they are actually different in code because bundle install has different arguments. You could make it free form but then it violates your desire for backwards compatibility and provides more potential for errors in setup.

So, the pull takes all of the above constraints in and makes available an additional bundler cli. I'm not sure how it could be done any different given the goals and constraints, but I'm open to ideas.

rosskevin commented 11 years ago

Hey Mark,

We might have different goals and I really need to knock out my tasks. Starting with this fork I'm going to maintain a separate repo under a new name to avoid confusion and add the things I need (all attributions and a pointer to this project will be included). I need to get what I need finished and move on to other things.

Thank you for the great start, feel free to pull anything I have back in as it will be Apache licensed. I really appreciate your hard work and understand that we might have different ends in mind. If it makes sense for you, please feel free to close this out without merging.

wolfeidau commented 11 years ago

Gday

I am working on merging this ATM albeit with a few tweaks.

Unfortunately github doesn't show you when i pull your repositor, i should have posted a note.

It is up to you if you want to fork it, not a lot I can do.