Skip to content
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

snap-repair: add uc20 support #9383

Merged
merged 10 commits into from Sep 29, 2020
Merged

Conversation

mvo5
Copy link
Collaborator

@mvo5 mvo5 commented Sep 21, 2020

This is a minimal change for snap-repair to support uc20. Some test refactor as well to make it easier to run tests with both a uc16 and uc20 setup.

This is a minimal change for snap-repair to support uc20. The tests
require a rework, ideally we would run all tests for a uc16 and
a uc20 model.
@mvo5 mvo5 added the UC20 label Sep 21, 2020
@mvo5 mvo5 added this to the 2.47 milestone Sep 21, 2020
@pedronis pedronis self-requested a review September 22, 2020 11:05
Copy link
Member

@anonymouse64 anonymouse64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The direction looks good to me, some comments

cmd/snap-repair/runner.go Outdated Show resolved Hide resolved
cmd/snap-repair/runner.go Outdated Show resolved Hide resolved
cmd/snap-repair/runner.go Outdated Show resolved Hide resolved
cmd/snap-repair/runner_test.go Outdated Show resolved Hide resolved
cmd/snap-repair/runner_test.go Show resolved Hide resolved
tests/core/basic20/task.yaml Outdated Show resolved Hide resolved
This commit keeps the version independent tests in runnerSuite.
It also uses testutil.BaseTest to simplify things.
The tests that deal with initState/writeSeedAssert now run with
both uc16/uc20 where it makes sense.

A new test runner20Suite.TestLoadStateInitDeviceInfoFail that checks
the failure conditions for modeenv reading is also added.
@mvo5 mvo5 changed the title [RFC] snap-repair: minimal uc20 support snap-repair: add uc20 support Sep 23, 2020
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments/considerations, I haven't looked closely at tests except to see that new code is covered

cmd/snap-repair/runner.go Outdated Show resolved Hide resolved
cmd/snap-repair/runner.go Outdated Show resolved Hide resolved
cmd/snap-repair/runner.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@mvo5 mvo5 added the Squash-merge Please squash this PR when merging. label Sep 28, 2020
Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Copy link
Collaborator

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

cmd/snap-repair/runner.go Outdated Show resolved Hide resolved
mvo5 and others added 2 commits September 28, 2020 15:18
Co-authored-by: Maciej Borzecki <maciek.borzecki@gmail.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Copy link
Member

@anonymouse64 anonymouse64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks for this

@@ -1778,3 +1669,218 @@ ls -l ${PATH##*:}/repair
c.Assert(err, IsNil)

}

// shared1620RunnerSuite is embedded by runner16Suite and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super super super super nitpicky, but this reads to me as "ubuntu core 1620" which is either 400 years old or 1600 years away in the future 😄

I would just rename this

Suggested change
// shared1620RunnerSuite is embedded by runner16Suite and
// sharedRunnerSuite is embedded by runner16Suite and

also afaik, the behavior on uc16 is the same as uc18, so really it's shared between all three variants

but again nitpick, no need to rename this unless you want to appease my nitpicks


const errPrefix = "cannot set device information: "
tests := []struct {
breakFunc func()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah very clever!

err = ioutil.WriteFile(dirs.SnapModeenvFile, mockModeenv, 0644)
c.Assert(err, IsNil)
// validate that modeenv is actually valid
_, err = boot.ReadModeenv("")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this

@mvo5 mvo5 merged commit 882d37e into snapcore:master Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Squash-merge Please squash this PR when merging.
Projects
None yet
5 participants