-
Notifications
You must be signed in to change notification settings - Fork 573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
state: add possible error return to TaskSet.Edge() #6733
Conversation
The current code in doUpdate works on a bunch of TaskSets like alias-updates, snap-updates and those are merged into a single TaskSet in the end. With that the TaskSetEdge information gets lost. This edge is import for the Remodel() code so we need to preserve it. This PR adds a new TaskSet.AddAllWithEdges() and uses it in doUpdate() to keep the edges informations in the flat TaskSet.
Co-Authored-By: mvo5 <michael.vogt@gmail.com>
As a followup to the discussion in we had in canonical#6599 this PR implements an error return in TaskSet.Edge().
overlord/state/task.go
Outdated
func (ts TaskSet) Edge(e TaskSetEdge) (*Task, error) { | ||
t, ok := ts.edges[e] | ||
if !ok { | ||
return nil, ErrNoState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ErrNoState doesn't carry a lot of information, but the usage of Edge will be mostly static (afawk) so I think
fmt.Errorf("internal error: no %q edge in task set", e)
would be maybe better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably slightly clearer with "missing" instead of "no"
overlord/devicestate/devicestate.go
Outdated
return nil, nil, nil, nil, fmt.Errorf("internal error: cannot find edge task in task set: %v", ts) | ||
edgeTask, err := ts.Edge(snapstate.DownloadAndChecksDoneEdge) | ||
if err != nil { | ||
return nil, nil, nil, nil, fmt.Errorf("internal error: cannot find edge task in task set: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then here we could just return err
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 with a question about the tests
overlord/state/task_test.go
Outdated
c.Assert(ts.Edge(edge2), IsNil) | ||
t, err := ts.Edge(edge1) | ||
c.Assert(t, IsNil) | ||
c.Assert(err, ErrorMatches, fmt.Sprintf("internal error: missing %q edge in task set", edge1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the repr strange for the edge? I would have used a `string` in the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
As a followup to the discussion in we had in
#6599
this PR implements an error return in TaskSet.Edge().
The relevant change is in 6426c3b