refactor: implement warnings as notices (#486)

This deletes the legacy warnings code and re-implements `pebble
warnings` and `pebble okay` using notices as warnings (as we originally
intended). The `/v1/warnings` endpoints have been removed, and `pebble
warnings` now hits `/v1/notices?types=warning`.

On every request, instead of returning the count of warnings and the
latest timestamp, we only return the timestamp of the latest (most
recent last-repeated time). This way the server doesn't have to track
which notices have been okayed -- the client can compare against its
stored timestamp and see if the server-reported timestamp is later.

So there are a few breaking changes in this PR:

* `Client.WarningsSummary() (count int, timestamp time.Time)` is now
`Client.LatestWarningTime() time.Time`, as there's no count anymore
* `GET /v1/warnings` (list warnings) is gone: `pebble warnings` now uses
`GET /v1/notices?types=warning`
* `POST /v1/warnings` (okay warnings) are gone: `pebble okay` now just
updates the local state file, and doesn't use the API/client at all
* Similarly, `Client.Warnings()` and `Client.Okay()` are gone; use
`Client.Notices()` and maintain local client-side state, respectively
* `State.Warnf` is still around, but it's now just a helper that calls
`State.AddNotice` with the right type (warning) and key (message)

Fixes #475.

client/client.go

@@ -147,9 +147,8 @@ type Config struct {
 type Client struct {
 	requester Requester
 
-	maintenance      error
-	warningCount     int
-	warningTimestamp time.Time
+	maintenance   error
+	latestWarning time.Time
 
 	getWebsocket getWebsocketFunc
 
@@ -219,11 +218,10 @@ func (client *Client) Maintenance() error {
 	return client.maintenance
 }
 
-// WarningsSummary returns the number of warnings that are ready to be shown to
-// the user, and the timestamp of the most recently added warning (useful for
-// silencing the warning alerts, and OKing the returned warnings).
-func (client *Client) WarningsSummary() (count int, timestamp time.Time) {
-	return client.warningCount, client.warningTimestamp
+// LatestWarningTime returns the most recent time a warning notice was
+// repeated, or the zero value if there are no warnings.
+func (client *Client) LatestWarningTime() time.Time {
+	return client.latestWarning
 }
 
 // RequestError is returned when there's an error processing the request.
@@ -377,8 +375,7 @@ func (rq *defaultRequester) Do(ctx context.Context, opts *RequestOptions) (*Requ
 
 	// Warnings are only included if not an error type response, so we don't
 	// replace valid local warnings with an empty state that comes from a failure.
-	rq.client.warningCount = serverResp.WarningCount
-	rq.client.warningTimestamp = serverResp.WarningTimestamp
+	rq.client.latestWarning = serverResp.LatestWarning
 
 	// Common response
 	return &RequestResponse{
@@ -447,16 +444,13 @@ func (client *Client) doAsync(method, path string, query url.Values, headers map
 // A response produced by the REST API will usually fit in this
 // (exceptions are the icons/ endpoints obvs)
 type response struct {
-	Result     json.RawMessage `json:"result"`
-	Status     string          `json:"status"`
-	StatusCode int             `json:"status-code"`
-	Type       string          `json:"type"`
-	Change     string          `json:"change"`
-
-	WarningCount     int       `json:"warning-count"`
-	WarningTimestamp time.Time `json:"warning-timestamp"`
-
-	Maintenance *Error `json:"maintenance"`
+	Result        json.RawMessage `json:"result"`
+	Status        string          `json:"status"`
+	StatusCode    int             `json:"status-code"`
+	Type          string          `json:"type"`
+	Change        string          `json:"change"`
+	LatestWarning time.Time       `json:"latest-warning"`
+	Maintenance   *Error          `json:"maintenance"`
 }
 
 // Error is the real value of response.Result when an error occurs.

client/client_test.go

@@ -340,3 +340,29 @@ func (cs *clientSuite) TestNonExistentSocketErrors(c *C) {
 	c.Check(notFoundErr.Path, Equals, "/tmp/not-the-droids-you-are-looking-for")
 	c.Check(notFoundErr.Err, NotNil)
 }
+
+func (cs *clientSuite) TestLatestWarningTime(c *C) {
+	cs.rsp = `{
+		"result": {
+			"version": "1.15.0",
+			"boot-id": "BOOTID"
+		},
+		"status": "OK",
+		"status-code": 200,
+		"type": "sync",
+		"latest-warning": "2018-09-19T12:44:19.680362867Z"
+	}`
+
+	info, err := cs.cli.SysInfo()
+	c.Assert(err, IsNil)
+	c.Check(info, DeepEquals, &client.SysInfo{
+		Version: "1.15.0",
+		BootID:  "BOOTID",
+	})
+	c.Check(cs.req.Method, Equals, "GET")
+	c.Check(cs.req.URL.Path, Equals, "/v1/system-info")
+
+	// this could be done at the end of any sync method
+	latest := cs.cli.LatestWarningTime()
+	c.Check(latest, Equals, time.Date(2018, 9, 19, 12, 44, 19, 680362867, time.UTC))
+}

client/notices.go

@@ -130,6 +130,10 @@ const (
 	// The key and data fields are provided by the user. The key must be in
 	// the format "example.com/path" to ensure well-namespaced notice keys.
 	CustomNotice NoticeType = "custom"
+
+	// Warnings are a subset of notices where the key is a human-readable
+	// warning message.
+	WarningNotice NoticeType = "warning"
 )
 
 type jsonNotice struct {

docs/reference/notices.md

@@ -20,14 +20,11 @@ In addition, a notice records optional *data* (string key-value pairs) from the
 
 These notice types are currently available:
 
-<!-- TODO: * `change-update`: recorded whenever a change is first spawned or its status is updated. The key for this type of notice is the change ID, and the notice's data includes the change `kind`. -->
+* `change-update`: recorded whenever a change is first spawned or its status is updated. The key for this type of notice is the change ID, and the notice's data includes the change `kind`.
 
 * `custom`: a custom client notice reported via `pebble notify`. The key and any data is provided by the user. The key must be in the format `example.com/path` to ensure well-namespaced notice keys.
 
-<!-- TODO: * `warning`: Pebble warnings are implemented in terms of notices. The key for this type of notice is the human-readable warning message.
-
-See comment at the top of internals/overlord/state/warning.go for more info.
--->
+* `warning`: Pebble warnings are implemented in terms of notices. The key for this type of notice is the human-readable warning message.
 
 ## Commands
 

internals/cli/cli.go

@@ -356,7 +356,11 @@ func Run(options *RunOptions) error {
 		return nil
 	}
 
-	maybePresentWarnings(cli.WarningsSummary())
+	state, err := loadCLIState(config.Socket)
+	if err != nil {
+		return fmt.Errorf("cannot load CLI state: %w", err)
+	}
+	maybePresentWarnings(state.WarningsLastListed, cli.LatestWarningTime())
 
 	return nil
 }
@@ -418,6 +422,9 @@ func getCopySource() string {
 type cliState struct {
 	NoticesLastListed time.Time `json:"notices-last-listed"`
 	NoticesLastOkayed time.Time `json:"notices-last-okayed"`
+
+	WarningsLastListed time.Time `json:"warnings-last-listed"`
+	WarningsLastOkayed time.Time `json:"warnings-last-okayed"`
 }
 
 type fullCLIState struct {

internals/cli/cli_test.go

@@ -56,8 +56,6 @@ func (s *BasePebbleSuite) SetUpTest(c *C) {
 	s.AddCleanup(cli.FakeIsStdoutTTY(false))
 	s.AddCleanup(cli.FakeIsStdinTTY(false))
 
-	os.Setenv("PEBBLE_LAST_WARNING_TIMESTAMP_FILENAME", filepath.Join(c.MkDir(), "warnings.json"))
-
 	oldConfigHome := os.Getenv("XDG_CONFIG_HOME")
 	s.AddCleanup(func() {
 		os.Setenv("XDG_CONFIG_HOME", oldConfigHome)
@@ -76,8 +74,6 @@ func (s *BasePebbleSuite) TearDownTest(c *C) {
 	cli.Stderr = os.Stderr
 	cli.ReadPassword = term.ReadPassword
 
-	os.Setenv("PEBBLE_LAST_WARNING_TIMESTAMP_FILENAME", "")
-
 	s.BaseTest.TearDownTest(c)
 }
 
@@ -141,7 +137,7 @@ func (s *PebbleSuite) TestErrorResult(c *C) {
 		fmt.Fprintln(w, `{"type": "error", "result": {"message": "cannot do something"}}`)
 	})
 
-	restore := fakeArgs("pebble", "warnings")
+	restore := fakeArgs("pebble", "notices")
 	defer restore()
 
 	err := cli.RunMain()
@@ -167,7 +163,7 @@ func (s *PebbleSuite) TestGetEnvPaths(c *C) {
 	c.Assert(socketPath, Equals, "/path/to/socket")
 }
 
-func (s *PebbleSuite) readCLIState(c *C) map[string]any {
+func (s *BasePebbleSuite) readCLIState(c *C) map[string]any {
 	data, err := os.ReadFile(s.cliStatePath)
 	c.Assert(err, IsNil)
 	var fullState map[string]any
@@ -187,7 +183,7 @@ func (s *PebbleSuite) readCLIState(c *C) map[string]any {
 	return v.(map[string]any)
 }
 
-func (s *PebbleSuite) writeCLIState(c *C, st map[string]any) {
+func (s *BasePebbleSuite) writeCLIState(c *C, st map[string]any) {
 	_, socketPath := cli.GetEnvPaths()
 	fullState := map[string]any{
 		"pebble": map[string]any{