Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

Move RhoCmd to its own file #49

Merged
merged 2 commits into from
Jul 7, 2017
Merged

Move RhoCmd to its own file #49

merged 2 commits into from
Jul 7, 2017

Conversation

noahl
Copy link

@noahl noahl commented Jul 7, 2017

Overall goal: get ready to import JBoss Scanner modules by making our
code use different Ansible modules.

This commit: one small move towards modularization.

Overall goal: get ready to import JBoss Scanner modules by making our
code use different Ansible modules.

This commit: one small move towards modularization.

"""RhoCmd: a base class with common functionality for our Ansible modules."""

import subprocess as sp
Copy link

Choose a reason for hiding this comment

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

I usually don't like this kind of renaming because it makes the code harder to understand and will require to check from where this is coming from. But I saw that it was present on the previous code so I think is fine to have this here. But we might want to avoid that in the future.

Copy link
Author

Choose a reason for hiding this comment

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

I strongly agree about not liking the renaming. I left it to keep this PR simpler, but I would like to go back and change it in the future.

Copy link

Choose a reason for hiding this comment

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

Fair enough, I agree.

@coveralls
Copy link

coveralls commented Jul 7, 2017

Coverage Status

Coverage remained the same at 47.791% when pulling 6e05d7d on issues/38-part-1 into 9121bcf on master.

Copy link

@elyezer elyezer left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@noahl noahl merged commit 490eb11 into master Jul 7, 2017
@noahl noahl deleted the issues/38-part-1 branch July 7, 2017 19:06
@chambridge chambridge mentioned this pull request Jul 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants