From 38cc1ad6d30e5ee310d6b397fa0a81ac5224f44c Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 21 Nov 2016 09:11:58 +0100 Subject: [PATCH 1/3] Show what will change in the "refresh-all" changes LP: #1643155 --- daemon/api.go | 7 ++----- daemon/api_test.go | 6 +++--- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/daemon/api.go b/daemon/api.go index b585eb85c6d..a4eb5479761 100644 --- a/daemon/api.go +++ b/daemon/api.go @@ -780,14 +780,11 @@ func snapUpdateMany(inst *snapInstruction, st *state.State) (msg string, updated } switch len(inst.Snaps) { - case 0: - // all snaps - msg = i18n.G("Refresh all snaps in the system") case 1: msg = fmt.Sprintf(i18n.G("Refresh snap %q"), inst.Snaps[0]) default: - quoted := make([]string, len(inst.Snaps)) - for i, name := range inst.Snaps { + quoted := make([]string, len(updated)) + for i, name := range updated { quoted[i] = strconv.Quote(name) } // TRANSLATORS: the %s is a comma-separated list of quoted snap names diff --git a/daemon/api_test.go b/daemon/api_test.go index 6ca13422de9..df03c425a06 100644 --- a/daemon/api_test.go +++ b/daemon/api_test.go @@ -2361,7 +2361,7 @@ func (s *apiSuite) TestPostSnapsOp(c *check.C) { st.Lock() defer st.Unlock() chg := st.Change(rsp.Change) - c.Check(chg.Summary(), check.Equals, "Refresh all snaps in the system") + c.Check(chg.Summary(), check.Equals, `Refresh snaps "fake1", "fake2"`) var apiData map[string]interface{} c.Check(chg.Get("api-data", &apiData), check.IsNil) c.Check(apiData["snap-names"], check.DeepEquals, []interface{}{"fake1", "fake2"}) @@ -2377,7 +2377,7 @@ func (s *apiSuite) TestRefreshAll(c *check.C) { snapstateUpdateMany = func(s *state.State, names []string, userID int) ([]string, []*state.TaskSet, error) { c.Check(names, check.HasLen, 0) t := s.NewTask("fake-refresh-all", "Refreshing everything") - return names, []*state.TaskSet{state.NewTaskSet(t)}, nil + return []string{"fake1", "fake2"}, []*state.TaskSet{state.NewTaskSet(t)}, nil } d := s.daemon(c) @@ -2387,7 +2387,7 @@ func (s *apiSuite) TestRefreshAll(c *check.C) { summary, _, _, err := snapUpdateMany(inst, st) st.Unlock() c.Assert(err, check.IsNil) - c.Check(summary, check.Equals, "Refresh all snaps in the system") + c.Check(summary, check.Equals, `Refresh snaps "fake1", "fake2"`) c.Check(refreshSnapDecls, check.Equals, true) } From 858cd9f179ab0bab1ee5081a26b1cd21e2c526a1 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 21 Nov 2016 09:32:01 +0100 Subject: [PATCH 2/3] generate meaningful message if refresh-all has no changes Stop-gap until we figure out what we really want here. Probably not generate a change at all if there is nothing to do. OTOH it might be useful to see in `snap changes` that auto-refresh is running (maybe :) --- daemon/api.go | 9 ++++++++- daemon/api_test.go | 23 +++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/daemon/api.go b/daemon/api.go index a4eb5479761..63514bbe048 100644 --- a/daemon/api.go +++ b/daemon/api.go @@ -779,7 +779,14 @@ func snapUpdateMany(inst *snapInstruction, st *state.State) (msg string, updated return "", nil, nil, err } - switch len(inst.Snaps) { + switch len(updated) { + case 0: + // not really needed but be paranoid + if len(inst.Snaps) != 0 { + return "", nil, nil, fmt.Errorf("internal error, when asking for a refresh of %q no update was found but not error was generated", inst.Snaps) + } + // FIXME: instead not generated a change(?) at all + msg = fmt.Sprintf(i18n.G("Refresh all snaps in the system found no updates")) case 1: msg = fmt.Sprintf(i18n.G("Refresh snap %q"), inst.Snaps[0]) default: diff --git a/daemon/api_test.go b/daemon/api_test.go index df03c425a06..a7adb18e75e 100644 --- a/daemon/api_test.go +++ b/daemon/api_test.go @@ -2391,6 +2391,29 @@ func (s *apiSuite) TestRefreshAll(c *check.C) { c.Check(refreshSnapDecls, check.Equals, true) } +func (s *apiSuite) TestRefreshAllNoChanges(c *check.C) { + refreshSnapDecls := false + assertstateRefreshSnapDeclarations = func(s *state.State, userID int) error { + refreshSnapDecls = true + return assertstate.RefreshSnapDeclarations(s, userID) + } + + snapstateUpdateMany = func(s *state.State, names []string, userID int) ([]string, []*state.TaskSet, error) { + c.Check(names, check.HasLen, 0) + return nil, nil, nil + } + + d := s.daemon(c) + inst := &snapInstruction{Action: "refresh"} + st := d.overlord.State() + st.Lock() + summary, _, _, err := snapUpdateMany(inst, st) + st.Unlock() + c.Assert(err, check.IsNil) + c.Check(summary, check.Equals, `Refresh all snaps in the system found no updates`) + c.Check(refreshSnapDecls, check.Equals, true) +} + func (s *apiSuite) TestRefreshMany(c *check.C) { refreshSnapDecls := false assertstateRefreshSnapDeclarations = func(s *state.State, userID int) error { From 16cc8a0685999369cd0db3ea47da7686583c16d2 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 24 Nov 2016 08:48:47 +0100 Subject: [PATCH 3/3] address review feedback (thanks John!) --- daemon/api.go | 6 +++--- daemon/api_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/daemon/api.go b/daemon/api.go index af60af2686b..f695c266d62 100644 --- a/daemon/api.go +++ b/daemon/api.go @@ -793,10 +793,10 @@ func snapUpdateMany(inst *snapInstruction, st *state.State) (msg string, updated case 0: // not really needed but be paranoid if len(inst.Snaps) != 0 { - return "", nil, nil, fmt.Errorf("internal error, when asking for a refresh of %q no update was found but not error was generated", inst.Snaps) + return "", nil, nil, fmt.Errorf("internal error: when asking for a refresh of %s no update was found but no error was generated", quotedNames(inst.Snaps)) } - // FIXME: instead not generated a change(?) at all - msg = fmt.Sprintf(i18n.G("Refresh all snaps in the system found no updates")) + // FIXME: instead don't generated a change(?) at all + msg = fmt.Sprintf(i18n.G("Refresh all snaps: no updates")) case 1: msg = fmt.Sprintf(i18n.G("Refresh snap %q"), inst.Snaps[0]) default: diff --git a/daemon/api_test.go b/daemon/api_test.go index 664f1e4dfdd..9867f13f211 100644 --- a/daemon/api_test.go +++ b/daemon/api_test.go @@ -2425,7 +2425,7 @@ func (s *apiSuite) TestRefreshAllNoChanges(c *check.C) { summary, _, _, err := snapUpdateMany(inst, st) st.Unlock() c.Assert(err, check.IsNil) - c.Check(summary, check.Equals, `Refresh all snaps in the system found no updates`) + c.Check(summary, check.Equals, `Refresh all snaps: no updates`) c.Check(refreshSnapDecls, check.Equals, true) }