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
seed/seedwriter: fail early when system seed directory exists #10220
seed/seedwriter: fail early when system seed directory exists #10220
Conversation
Fail early when creating a Core 20 seed when the target directory exists. This helps to prevent a situation when creating seed gets interrupted and gets restarted. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
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
s.opts.Label = "20191003" | ||
for _, t := range tests { | ||
baseLabel := "20191003" | ||
for idx, t := range tests { |
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.
we could also remove the dir instead? I don't have a very strong preference but wanted to mention it
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.
using the index as a suffix seems reasonable enough to me
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.
a couple of questions (the one about the tests is not a blocker)
…er-fail-when-dir-is-present
Seedwriter now fails early when the system directory already exists Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
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.
I would prefer a dedicate error
…edwriter-fail-when-dir-is-present
…ry already exists Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
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.
looks good but error doesn't follow naming conventions
seed/seedwriter/writer.go
Outdated
@@ -421,12 +421,28 @@ func (w *Writer) SetOptionsSnaps(optSnaps []*OptionsSnap) error { | |||
return nil | |||
} | |||
|
|||
// ErrSystemAlreadyExists is an error returned when given seed system already | |||
// exists. | |||
type ErrSystemAlreadyExists struct { |
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.
error structs need to be called SystemAlreadyExistsError
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
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.
thanks
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.
still lgtm
s.opts.Label = "20191003" | ||
for _, t := range tests { | ||
baseLabel := "20191003" | ||
for idx, t := range tests { |
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.
using the index as a suffix seems reasonable enough to me
Feels like restarting the spread jobs for the 3rd time is wasteful. @mvo5 can you merge this PR? The failure on 14.04 is unrelated. |
Fail early when creating a Core 20 seed when the target directory exists. This
helps to prevent a situation when creating seed gets interrupted and gets
restarted.