Make rebot call the Reboot API

[robertodauria]

This PR makes rebot call the Reboot API instead of the deprecated drac.py.

A mix of abstraction and "monkey patching" has been used to achieve 100% test coverage in the new reboot package and an equivalent coverage for main.go (86%).

---

This change is 

[stephen-soltesz]

Reviewable status: 0 of 5 files reviewed, 6 unresolved discussions (waiting on @evfirerob and @stephen-soltesz)

---

main.go, line 39 at r1 (raw file):

	defaultCredentialsPath = "/tmp/credentials"
	defaultHistoryPath     = "/tmp/candidateHistory.json"
	defaultPrometheusAddr  = "prometheus.mlab-sandbox.measurementlab.net"

Should the project be a parameter?

---

main.go, line 223 at r1 (raw file):

	flag.StringVar(&rebootAddr, "reboot.addr", "",
		"Reboot API instance to send reboot request to.")
	flag.StringVar(&rebootUsername, "reboot.username", "",

Rebot uses two different mechanisms to discovery the user/pass for prometheus and the user/pass for the reboot api. Could it use the same mechanism?

---

main.go, line 257 at r1 (raw file):

	// Create the HTTPRebooter.
	baseURL := rebootAddr + rebootEndpoint

The rebootEndpoint feels like it's a property of the NewHTTPRebooter interface that does not need to be exposed to the user of the rebooter interface.

What do you think of moving this constant to the reboot package?

---

reboot/reboot.go, line 31 at r1 (raw file):
Would it be equivalent to say:

	newHTTPRequest = http.NewRequest

Where else can this be used?

---

reboot/reboot.go, line 82 at r1 (raw file):

	if err != nil {
		log.WithError(err).Error("Cannot send reboot request.")
		metricRebootRequests.WithLabelValues(toReboot.Name, toReboot.Site, "reboot", "failure").Add(1)

Would it be helpful to distinguish these failure cases like "failure-request", "failure-read", "failure-status", etc?

---

reboot/reboot_test.go, line 36 at r1 (raw file):

	client := NewTestClient(func(req *http.Request) *http.Response {
		fmt.Println(req.Header.Get("Authorization"))
		if req.Header.Get("Authorization") != "Basic dXNlcjpwYXNz" {

Please add a comment about this basic auth string, something like $ echo -n user:pass | base64 would be enough. It's hard to know how to verify this without some extra hints for the reader.

Above all other code, unit tests must be "correct by inspection". Writing them to be "obviously correct" is the goal.

[robertodauria]

Reviewable status: 0 of 5 files reviewed, 6 unresolved discussions (waiting on @stephen-soltesz)

---

main.go, line 39 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Should the project be a parameter?

Done. Instead of -prometheus.addr we will need to specify -project and the URL is built according to that.

---

main.go, line 223 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Rebot uses two different mechanisms to discovery the user/pass for prometheus and the user/pass for the reboot api. Could it use the same mechanism?

Done. I've added -prometheus.username and -prometheus.password and removed the code to get credentials from a file.

---

main.go, line 257 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

The rebootEndpoint feels like it's a property of the NewHTTPRebooter interface that does not need to be exposed to the user of the rebooter interface.

What do you think of moving this constant to the reboot package?

Done. Makes sense, definitely an oversight.

---

reboot/reboot.go, line 31 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Would it be equivalent to say:

	newHTTPRequest = http.NewRequest

Where else can this be used?

Done.

---

reboot/reboot.go, line 82 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Would it be helpful to distinguish these failure cases like "failure-request", "failure-read", "failure-status", etc?

Done.

---

reboot/reboot_test.go, line 36 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Please add a comment about this basic auth string, something like $ echo -n user:pass | base64 would be enough. It's hard to know how to verify this without some extra hints for the reader.

Above all other code, unit tests must be "correct by inspection". Writing them to be "obviously correct" is the goal.

Done. Thanks!

[stephen-soltesz]

:

Reviewed 1 of 5 files at r1, 4 of 4 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved

---

main.go, line 223 at r1 (raw file):

Previously, evfirerob (Roberto D'Auria) wrote…

Done. I've added -prometheus.username and -prometheus.password and removed the code to get credentials from a file.

Now that you're using ArgsFromEnv these could all be environment variables too. 👍