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

Client for Executing Commands #103

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@ashcrow
Member

ashcrow commented Oct 1, 2018

Requires #100

Added a test as an example. The stub probably should live in it's own file.

Note: The 1st commit is #100.

ashcrow added some commits Sep 28, 2018

Turn rpm-ostree code into a client
This change allows for modification of rpm-ostree related code so
unittesting can occur. Instead of the functionality being spread
through private functions within the daemon package, a structure
which implements NodeUpdaterClient is used and is passed in to
the Daemon at creation.

Signed-off-by: Steve Milner <smilner@redhat.com>
Add ProcessClient for use with testing
Also included the start of rpm-ostree code using ProcessClient.

Signed-off-by: Steve Milner <smilner@redhat.com>
@openshift-ci-robot

This comment has been minimized.

Show comment
Hide comment
@openshift-ci-robot

openshift-ci-robot Oct 1, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot commented Oct 1, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 1, 2018

Member

/hold

Member

ashcrow commented Oct 1, 2018

/hold

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow
Member

ashcrow commented Oct 1, 2018

@jlebon

This comment has been minimized.

Show comment
Hide comment
@jlebon

jlebon Oct 1, 2018

Member

Hmm, given how thin the rpm-ostree wrapper is, I'm not sure if it's worth adding another abstration layer here too. I.e. we're more interested with how the rest of the MCD interacts with the NodeUpdateClient (i.e. #100) than how rpm-ostree deserializes JSON, right?

Member

jlebon commented Oct 1, 2018

Hmm, given how thin the rpm-ostree wrapper is, I'm not sure if it's worth adding another abstration layer here too. I.e. we're more interested with how the rest of the MCD interacts with the NodeUpdateClient (i.e. #100) than how rpm-ostree deserializes JSON, right?

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 1, 2018

Member

We're not really interested in how rpm-ostree deserializes json so much as we're interested in that the functions/methods react properly when results are produced. For example, with TestGetBootedDeployment we would need to continue adding to the test to ensure that the deployment matching (or lack there of) produces the proper results. If a change is made and the test starts failing we know that the internal contract is broken and we must refactor the code to honor the contract, or change the contract by updating the test.

Member

ashcrow commented Oct 1, 2018

We're not really interested in how rpm-ostree deserializes json so much as we're interested in that the functions/methods react properly when results are produced. For example, with TestGetBootedDeployment we would need to continue adding to the test to ensure that the deployment matching (or lack there of) produces the proper results. If a change is made and the test starts failing we know that the internal contract is broken and we must refactor the code to honor the contract, or change the contract by updating the test.

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 1, 2018

Member

But it's still a valid note @jlebon. We could alternatively not worry about the NodeUpdateClient for testing, but instead test from that level above by stubbing the NodeUpdateClient in a similar way shown with the stub in this PR.

Member

ashcrow commented Oct 1, 2018

But it's still a valid note @jlebon. We could alternatively not worry about the NodeUpdateClient for testing, but instead test from that level above by stubbing the NodeUpdateClient in a similar way shown with the stub in this PR.

@jlebon

This comment has been minimized.

Show comment
Hide comment
@jlebon

jlebon Oct 1, 2018

Member

We're not really interested in how rpm-ostree deserializes json so much as we're interested in that the functions/methods react properly when results are produced.

As far as the rest of the MCD is concerned, we can already test this with just #100, right? What we're adding here is to test rpm-ostree.go itself? I guess I don't see enough business logic there to warrant another harness. (Not to mention it'll get even thinner once the installer learns to pivot).

We could alternatively not worry about the NodeUpdateClient for testing, but instead test from that level above by stubbing the NodeUpdateClient in a similar way shown with the stub in this PR.

Yeah exactly, that's more where I thought #100 was going!

Anyway, not diametrically opposed, just not sure about the benefit/cost ratio.

Member

jlebon commented Oct 1, 2018

We're not really interested in how rpm-ostree deserializes json so much as we're interested in that the functions/methods react properly when results are produced.

As far as the rest of the MCD is concerned, we can already test this with just #100, right? What we're adding here is to test rpm-ostree.go itself? I guess I don't see enough business logic there to warrant another harness. (Not to mention it'll get even thinner once the installer learns to pivot).

We could alternatively not worry about the NodeUpdateClient for testing, but instead test from that level above by stubbing the NodeUpdateClient in a similar way shown with the stub in this PR.

Yeah exactly, that's more where I thought #100 was going!

Anyway, not diametrically opposed, just not sure about the benefit/cost ratio.

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 3, 2018

Member

Let's keep this on hold and revisit it after #100 and others. This PR serves as an example of how to use the clients for testing, but it doesn't have nearly as much value as the other PRs for clients and testing.

Member

ashcrow commented Oct 3, 2018

Let's keep this on hold and revisit it after #100 and others. This PR serves as an example of how to use the clients for testing, but it doesn't have nearly as much value as the other PRs for clients and testing.

@ashcrow ashcrow closed this Oct 9, 2018

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