store: add Store.SnapAction to support the new install/refresh api endpoint #4771
Conversation
|
based on #4770 |
pedronis
force-pushed
the
store-install-refresh
branch
from
c36efce
to
5c26dbc
Compare
pedronis
force-pushed
the
store-install-refresh
branch
from
a68bd4b
to
b3e4e62
Compare
Codecov Report
@@ Coverage Diff @@
## master #4771 +/- ##
==========================================
+ Coverage 78.97% 78.98% +0.01%
==========================================
Files 477 478 +1
Lines 34763 35045 +282
==========================================
+ Hits 27455 27682 +227
- Misses 5120 5163 +43
- Partials 2188 2200 +12
Continue to review full report at Codecov.
|
store/store.go
Outdated
| // installed snaps in installedCtxt. If the request was overall | ||
| // successul (200) but there were reported errors it will return both | ||
| // the snap infos and an InstallRefreshError. | ||
| func (s *Store) InstallRefresh(installedCtxt []*CurrentSnap, actions []*InstallRefreshAction, user *auth.UserState, opts *RefreshOptions) ([]*snap.Info, error) { |
There was a problem hiding this comment.
any reason not to give this a context.Context, to make it cancelable?
There was a problem hiding this comment.
this is done, it was necessary also to integrate with other current work
pedronis
changed the title
store/store.go
Outdated
|
|
||
| const ( | ||
| apiV1 apiLevel = 0 | ||
| apiV2 apiLevel = 1 |
There was a problem hiding this comment.
That's quite misleading. Why is that not V0 and V1 respectively, or V1 = 1 and V2 = 2?
store/store.go
Outdated
| Fields []string `json:"fields"` | ||
| } | ||
|
|
||
| type snapActionResp struct { |
There was a problem hiding this comment.
Resp vs. Result isn't clear. They both mean the same thing. Would ResultList be correct here?
store/errors.go
Outdated
| if len(e.Other) > 0 { | ||
| es = append(es, "other install/refresh errors:") | ||
| for _, e := range e.Other { | ||
| es = append(es, fmt.Sprintf(" - %v", e)) |
There was a problem hiding this comment.
The format here feels a bit verbose and perhaps unfriendly to read.
Perhaps we can separate this in two cases: if there is only one error, whatever it is, let's spell it out. For example:
cannot refresh snap %q: %v"
This will probably be the most common case, so it's nice to make it very friendly.
Then, if there are more problems, we can generalize the message:
cannot refresh snaps:
snap %q: %s
snap %q: %s
snap %q: %s
No need for the initial "-". The first word is already the leading sign.
The first sentence would change according to the errors, but it would be a single message. One of these:
cannot install snaps:cannot refresh snaps:cannot install or refresh snaps:
|
squashed merged the next bit instead to reduce the number of PRs that we will need to backport |
This is quite large, most of it is additions to store_test.go to cover the new code.
See pedronis@f3914cd for a preview of how it's going to be used.
Support for macaroon refreshes will be in its own follow up PR. It needs to be there when we start using this though.