Skip to content

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Jan 4, 2014

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 4da970b on Fryguy:extract_run_into_awesome_spawn into 385ee8c on ManageIQ:master.

@Fryguy
Copy link
Member Author

Fryguy commented Jan 4, 2014

@brandondunne Please review.

@bdunne
Copy link
Member

bdunne commented Jan 6, 2014

Looks good 👍

@bdunne
Copy link
Member

bdunne commented Jan 6, 2014

@jrafanie Any concerns?

@jrafanie
Copy link
Member

jrafanie commented Jan 6, 2014

@brandondunne We need to cut a major version because it introduces a new dependency and the namespacing changed:

CommandResultError          -> AwesomeSpawn::CommandResultError
CommandResult               -> AwesomeSpawn::CommandResult
LinuxAdmin::NoSuchFileError -> AwesomeSpawn::NoSuchFileError

We'll need to add entry to the changelog in case anyone is referencing these classes directly.

When we're ready to merge this, we should cut a tag and change the version, changelog, etc.

Copy link
Member

Choose a reason for hiding this comment

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

@Fryguy Why did you change from :run to :run! ?

Copy link
Member Author

Choose a reason for hiding this comment

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

So the old specs were relying on an implementation detail of the old run code, specifically that run! called run. That is no longer the right assumption, so the tests were failing. I changed them to properly expect the method they are calling.

@jvlcek
Copy link
Member

jvlcek commented Jan 22, 2014

@Fryguy, Thanks for the replies. Looks good to me. More educational for me than anything...

@Fryguy
Copy link
Member Author

Fryguy commented Jan 24, 2014

@brandondunne What's left on this? This is going to have a conflict with #87 , hence why I'm asking

@bdunne
Copy link
Member

bdunne commented Jan 24, 2014

@Fryguy Code-wise everything looks great. I think we just need to discuss merging it with @jrafanie. We may want to merge #87 (and a couple others) first.

@jrafanie
Copy link
Member

Yeah, @Fryguy @brandondunne, let's get any nonbreaking changes in first, cut a patchlevel tag bump and then merge this and bump major/minor.

@Fryguy
Copy link
Member Author

Fryguy commented Feb 3, 2014

@brandondunne Going to change the awesome_spawn version since it has bumped since I started this PR...hold off on merge.

@Fryguy
Copy link
Member Author

Fryguy commented Feb 3, 2014

Ok, updated to latest awesome_spawn version and also changed Rpm.import_key spec to fall in line with the changes here.

bdunne added a commit that referenced this pull request Feb 10, 2014
@bdunne bdunne merged commit b7c0c9b into ManageIQ:master Feb 10, 2014
@bdunne bdunne deleted the extract_run_into_awesome_spawn branch February 10, 2014 19:49
@Fryguy
Copy link
Member Author

Fryguy commented Feb 10, 2014

yay! 🍔

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