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 a Puppet backend #38

Merged
merged 6 commits into from Apr 6, 2013

Conversation

Projects
None yet
2 participants
@raphink
Copy link
Contributor

raphink commented Apr 5, 2013

This depends on #36

Note: the rspec tests for this backend are currently missing.

raphink added some commits Apr 5, 2013

Use classes for backends
  Use an helper for backends again
  Instantiate backends with commands
  Remove specific implementations from matchers

@mizzy mizzy merged commit 103855a into mizzy:master Apr 6, 2013

1 check passed

default The Travis build passed
Details
@mizzy

This comment has been minimized.

Copy link
Owner

mizzy commented Apr 6, 2013

This is very interesting idea.

But serverspec should work without installation of Puppet.
So I merged this request and removed dependency on Puppet from serverspec core.

@mizzy

This comment has been minimized.

Copy link
Owner

mizzy commented Apr 6, 2013

And It's possible that I will remove Puppet backend in the future.

@mizzy

This comment has been minimized.

Copy link
Owner

mizzy commented Apr 6, 2013

I found that your commits breaks the compatibility.

Also your commits are highly abstracted and complex.
I'd like to keep serverspec code base simple.

So I will revert your commits.Sorry,

mizzy added a commit that referenced this pull request Apr 6, 2013

@raphink

This comment has been minimized.

Copy link
Contributor

raphink commented Apr 6, 2013

OK. I can do without the PR 38 as I could provide the backend myself, but I need PR 36 in order to able to plug a backend that does not use commands. The only abstraction it adds is making backends into classes so that you're not limited to launching commands for tests. If this can't be merged, I'll have to resort to forking the code base, cause there's no way I can use the Puppet backend cleanly otherwise.

@mizzy

This comment has been minimized.

Copy link
Owner

mizzy commented Apr 6, 2013

With PR 36, I found that be_running matcher's behavior changed. Before PR 36, it checks with commands.check_running() and commands.check_process. But with PR 36, it uses only check_running(). This breaks backward compatibility.

I thought there might be another compatibility breaker.So I reverted.

If you can implement them without breaking backward compatibility, I will accept it.

@raphink

This comment has been minimized.

Copy link
Contributor

raphink commented Apr 6, 2013

Hmmm no, PR 36 still uses both commands check_running and check_process:

raphink@739ff34#L2R74

The difference is that it calls backend.check_running, and in the case of the Exec/Ssh backends, the implementation happens to call both commands.check_running and commands.check_process. Calling both commands is an implementation matter, not a backend one, which is why the matcher only calls backend.check_running. Some backends (the Puppet one for example) don't need to call check_process in their implementation.

Note that all specs passed with both PR 36 and 38, so if compatibility was broken, it would probably be missing specs.

@raphink

This comment has been minimized.

Copy link
Contributor

raphink commented Apr 6, 2013

About complexity, as surprising as it might seem, the goal of these PRs is precisely to make things more simple: matchers only call backend.check_something, making the code as simple as can be, since all the implementation complexity is moved to the backend classes.

@mizzy

This comment has been minimized.

Copy link
Owner

mizzy commented Apr 6, 2013

Hmmm no, PR 36 still uses both commands check_running and check_process:

raphink/serverspec@739ff34#L2R74

Oh, I missed that. Sorry.I understand that you care backward compatibility.
Sorry for my misunderstanding.

And I'd like to try merge your PRs again.

Thanks.

mizzy added a commit that referenced this pull request Apr 6, 2013

Revert "Revert pull req #36, #37, #38"
This reverts commit 5df4e16.
@raphink

This comment has been minimized.

Copy link
Contributor

raphink commented Apr 6, 2013

Thank you very much @mizzy :-)

If you find any issues, please let me know and I'll do my best to help with them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment