Turn rpm-ostree code into a client #100

ashcrow · 2018-09-28T18:50:54Z

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 DeploymentClient is used and is passed in to
the Daemon at creation.

Part of #101

/cc @jlebon @sdemos @kikisdeliveryservice

ashcrow · 2018-09-28T18:51:11Z

/hold

I'd like to wait on merging things like this until after the current round of testing.

kikisdeliveryservice · 2018-09-28T20:55:40Z

I'm really excited about these PRs (and future unit testing). Thanks for getting this started!

ashcrow · 2018-10-01T14:27:28Z

Deployment is also an rpm-ostree component. I fear there will be more collisions in names like this. I am, however, good with prefixing with something like NodeDeploymentClient or HostDeploymentClient.

@abhinavdahiya thoughts?

jlebon · 2018-10-01T15:16:39Z

Hmm yeah, I think that makes sense overall for the goal of unit testing. That said for upstream CI, hopefully we can get a real cluster going and test there?

I am, however, good with prefixing with something like NodeDeploymentClient or HostDeploymentClient.

Or maybe something like NodeUpdater?

ashcrow · 2018-10-01T15:35:43Z

Or maybe something like NodeUpdater?

I'm 👍 to that. I'll make the change in a bit.

ashcrow · 2018-10-01T15:51:00Z

Hmm yeah, I think that makes sense overall for the goal of unit testing. That said for upstream CI, hopefully we can get a real cluster going and test there?

Agreed, but that's a different set of work :-)

ashcrow · 2018-10-01T16:01:06Z

Updated.

sdemos · 2018-10-01T18:42:17Z

pkg/daemon/rpm-ostree.go

+}
+
+// GetBootedDeployment returns the current deployment found
+func (r *RpmOstreeClient) GetBootedDeployment(rootMount string) (*RpmOstreeDeployment, error) {


is this function called by anything external to this package? I only see the one change. if not, I would probably advocate for removing it from the client interface and just having it as a method on RpmOstreeClient.

/cc @jlebon

Yup, that sounds good to me!

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>

ashcrow · 2018-10-04T16:25:04Z

Should test implementations be in the same file as the main implementation or should they be implemented within a $NAME_test.go with the understanding other $NAME_test.gos will use them as well?

jlebon · 2018-10-04T19:41:44Z

Code looks sane to me!

Should test implementations be in the same file as the main implementation or should they be implemented within a $NAME_test.go with the understanding other $NAME_test.gos will use them as well?

I'll let folks more experienced with golang decide.

ashcrow · 2018-10-04T20:23:42Z

FWIW my inclination is to put the test implementations in $NAME_test.go files to keep the main implementation files easy to follow.

ashcrow · 2018-10-04T20:37:16Z

In terms of testing usage. This would look something like this (untested, uncompiled, but close :-)):

testClient := RpmOstreeClientMock{
	GetBootedOSImageURLReturns: []GetBootedOSImageURLReturn{
            OsImageURL: "theurl",
            Version: "some-version",
            Error: nil,
        }
	RunPivotReturns: []error{nil, fmt.Errorf("broken")}
}

// Create an instance that uses the client
// Execute a method that calls GetBootedOSImageURL off the client. It will get the contents past to GetBootedOSImageURLReturns every time it is executed.
// Execute a method that calls RunPivot. The first time it will return no error. The second run and beyond will return an the "broken" error.

ashcrow · 2018-10-04T20:49:44Z

/hold cancel

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

ashcrow · 2018-10-08T15:50:47Z

pkg/daemon/update_test.go

+	}
+
+	// Create a Daemon instance with mocked clients
+	d := Daemon{


I would expect creating a test Daemon instance would likely be added as well to keep many of the tests from recreating the instances each time.

ashcrow · 2018-10-08T15:51:16Z

Moved test code into *_test.go and added an example test for update.go.

ashcrow · 2018-10-08T15:51:23Z

@sdemos PTAL

ashcrow · 2018-10-08T18:15:20Z

FWIW

Running tool: /usr/bin/go test -timeout 30s github.com/openshift/machine-config-operator/pkg/daemon -run ^TestUpdateOS$

ok  	github.com/openshift/machine-config-operator/pkg/daemon	0.007s
Success: Tests passed.

sdemos

this all looks good to me. I'll let @jlebon give the final go-ahead since it hits on the rpm-ostree code.

sdemos · 2018-10-08T19:42:02Z

just kidding, he is out today, I'll just do it.

/lgtm

openshift-ci-robot · 2018-10-08T19:42:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, sdemos

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:

~~cmd/machine-config-daemon/OWNERS~~ [ashcrow,sdemos]
~~pkg/daemon/OWNERS~~ [ashcrow,sdemos]

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 requested review from jlebon, kikisdeliveryservice and sdemos September 28, 2018 18:50

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 28, 2018

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2018

ashcrow force-pushed the mcd-deployment-client branch from 0c98546 to 99484d7 Compare October 1, 2018 16:00

sdemos reviewed Oct 1, 2018

View reviewed changes

ashcrow mentioned this pull request Oct 1, 2018

Client for Executing Commands #103

Closed

ashcrow force-pushed the mcd-deployment-client branch 2 times, most recently from f10d421 to 4732d45 Compare October 4, 2018 16:01

ashcrow force-pushed the mcd-deployment-client branch from 4732d45 to f9f1c6f Compare October 4, 2018 16:22

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2018

ashcrow force-pushed the mcd-deployment-client branch from f9f1c6f to bfc5b82 Compare October 8, 2018 15:14

ashcrow added 2 commits October 8, 2018 11:48

daemon/rpm-ostree_test.go: Add test structures

cbb9adf

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

daemon/update_test.go: Initial example of mock testing

ef2b688

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

ashcrow force-pushed the mcd-deployment-client branch from bfc5b82 to ef2b688 Compare October 8, 2018 15:49

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 8, 2018

ashcrow commented Oct 8, 2018

View reviewed changes

sdemos approved these changes Oct 8, 2018

View reviewed changes

openshift-ci-robot assigned sdemos Oct 8, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2018

openshift-merge-robot merged commit c0a19e0 into openshift:master Oct 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn rpm-ostree code into a client #100

Turn rpm-ostree code into a client #100

ashcrow commented Sep 28, 2018 •

edited

ashcrow commented Sep 28, 2018

kikisdeliveryservice commented Sep 28, 2018

ashcrow commented Oct 1, 2018 •

edited

jlebon commented Oct 1, 2018

ashcrow commented Oct 1, 2018

ashcrow commented Oct 1, 2018

ashcrow commented Oct 1, 2018

sdemos Oct 1, 2018

ashcrow Oct 1, 2018

jlebon Oct 1, 2018

ashcrow Oct 4, 2018

ashcrow commented Oct 4, 2018

jlebon commented Oct 4, 2018

ashcrow commented Oct 4, 2018

ashcrow commented Oct 4, 2018 •

edited

ashcrow commented Oct 4, 2018

ashcrow Oct 8, 2018

ashcrow commented Oct 8, 2018

ashcrow commented Oct 8, 2018

ashcrow commented Oct 8, 2018

sdemos left a comment

sdemos commented Oct 8, 2018

openshift-ci-robot commented Oct 8, 2018

Turn rpm-ostree code into a client #100

Turn rpm-ostree code into a client #100

Conversation

ashcrow commented Sep 28, 2018 • edited

ashcrow commented Sep 28, 2018

kikisdeliveryservice commented Sep 28, 2018

ashcrow commented Oct 1, 2018 • edited

jlebon commented Oct 1, 2018

ashcrow commented Oct 1, 2018

ashcrow commented Oct 1, 2018

ashcrow commented Oct 1, 2018

sdemos Oct 1, 2018

Choose a reason for hiding this comment

ashcrow Oct 1, 2018

Choose a reason for hiding this comment

jlebon Oct 1, 2018

Choose a reason for hiding this comment

ashcrow Oct 4, 2018

Choose a reason for hiding this comment

ashcrow commented Oct 4, 2018

jlebon commented Oct 4, 2018

ashcrow commented Oct 4, 2018

ashcrow commented Oct 4, 2018 • edited

ashcrow commented Oct 4, 2018

ashcrow Oct 8, 2018

Choose a reason for hiding this comment

ashcrow commented Oct 8, 2018

ashcrow commented Oct 8, 2018

ashcrow commented Oct 8, 2018

sdemos left a comment

Choose a reason for hiding this comment

sdemos commented Oct 8, 2018

openshift-ci-robot commented Oct 8, 2018

ashcrow commented Sep 28, 2018 •

edited

ashcrow commented Oct 1, 2018 •

edited

ashcrow commented Oct 4, 2018 •

edited