cmd/snap: fix snap run command auto completion #11315
Conversation
LP: #1844776
anonymouse64
changed the title
Codecov Report
@@ Coverage Diff @@
## master #11315 +/- ##
==========================================
+ Coverage 78.30% 78.31% +0.01%
==========================================
Files 930 930
Lines 106430 106464 +34
==========================================
+ Hits 83344 83382 +38
+ Misses 17908 17902 -6
- Partials 5178 5180 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
…/snap Also add unit tests for the two situations from main() of cmd/snap/main.go that have special handling since we apparently did not have any unit test coverage there before. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
|
Thanks for working on this, it seems we have some missing unit tests for cmd/snap/main.go, hence why you didn't catch or notice that this doesn't work for the case where snap run is executed via the symlinks from Let me know if you would like me to push this commit to this branch here |
Yes, please. Thanks |
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
|
|
||
| func (s installedCommand) Complete(match string) []flags.Completion { | ||
| cli := mkClient() | ||
| apps, err := cli.Apps(nil, client.AppOptions{Service: false}) |
There was a problem hiding this comment.
The only downside I see is that this will make snap do and load snap.yaml from each installed snap. Maybe we should consider being smarter completion of apps at some point, similarly to what we have for snap advise. In any way, this would be a followup material as it would probably need some discussion.
There was a problem hiding this comment.
True, it is loading snap.yaml from every installed snap, but it is only doing this when it needs to complete
|
@patriciadomin There were some conflicts with the master branch, so I merged master to this branch and pushed to your repo, hope it's ok. |
| snaps := map[string]int{} | ||
| var ret []flags.Completion | ||
| for _, app := range apps { | ||
| if app.IsService() { | ||
| continue | ||
| } | ||
| name := snap.JoinSnapApp(app.Snap, app.Name) | ||
| if !strings.HasPrefix(name, match) { | ||
| continue | ||
| } | ||
| ret = append(ret, flags.Completion{Item: name}) | ||
| if len(match) <= len(app.Snap) { | ||
| snaps[app.Snap]++ | ||
| } | ||
| } | ||
| for snap, n := range snaps { | ||
| if n > 1 { | ||
| ret = append(ret, flags.Completion{Item: snap}) | ||
| } | ||
| } |
There was a problem hiding this comment.
AFAICT, there are no tests checking the completion logic. Could we write some?
| c.Assert(r.Method, check.Equals, "GET") | ||
| EncodeResponseBody(c, w, map[string]interface{}{ | ||
| "type": "sync", | ||
| "result": fortestingConnectionList, |
There was a problem hiding this comment.
I think this needs to return something that the completion logic can iterate through. As it is, this will fail to be deserialized and not exercise the completion logic
|
Hi @patriciadomin, this work is still useful to us, would you like to continue? |
LP: #1844776