Make .configure() configure network settings for both 1.x & 2.x #25

pimterry · 2017-10-27T12:27:41Z

Connects to balena-io/balena-cli#494
Connects to balena-io/balena-cli#537

This PR does a lot of exciting things. Primarily, it ensures that resin-device-init knows how to initialize a device's network, not just its config.json. Lots of the logic to do that comes from the reconfix test framework's schemas, and the existing logic to do this for ResinOS 2.x images in the CLI (which will be removed shortly to start using this instead).

On top of that, this upgrades to resin-image-fs v4 (since that's a prerequisite), adds logic for OS version detection (some context in flowdock) since we need that to decide which type of network config to do, and updates the npm build script to always rebuild the docs, and build before you test, since that's easy to forget otherwise.

Some of this is arguably a little hacky, though I've tried to minimize that. A lot of that is due to limitations and outright bugs in the current reconfix implementation. I'm just working around those for now, since the JS reconfix is deprecated, and we should be replacing those parts with Rust reconfix soon-ish hopefully.

This isn't totally done - I still need to add some proper tests for this, but those are slightly tricky. It'd be useful if you could start on reviewing the implementation logic in the meantime though, and I'll patch some add some more thorough tests for this later too.

zvin · 2017-10-30T10:39:04Z

lib/init.coffee

+				if not majorVersion? || majorVersion == 2
+					network.configureOS2Network(image, manifest, options)
+				if not majorVersion? || majorVersion == 1
+					network.configureOS1Network(image, manifest, options)


This block (the one that configures network) should use Promise.all or Promise.join to we wait for both operations to complete.

Or maybe it should use thens . I don't know if reconfix suppports doing things in parallel.

Yep, it'll do undefined things if you try to write changes in parallel. I've made it wait on these synchronously instead.

zvin · 2017-10-30T10:42:03Z

lib/network.coffee

+
+_ = require('lodash')
+path = require('path')
+utils = require('./utils')


NIT: can we put local imports after the other ones?

zvin · 2017-10-30T10:44:35Z

lib/network.coffee

+		contents.files ?= {}
+		contents.files['network/network.config'] ?= ''
+		imagefs.writeFile(configFilePath, JSON.stringify(contents))
+	.catch fileNotFoundError, ->


I think you can use .catch { code: 'ENOENT' }, -> directly with Bluebird.

If this catch is here to catch errors of imagefs.readFile(configFilePath), can we put it right after it (before JSON.parse)?

I think you can use .catch { code: 'ENOENT' }, -> directly with Bluebird.

Sadly, this is using the promise returned from resin-image-fs, which uses an old version of bluebird that can't do this, and throws https://github.com/petkaantonov/bluebird/wiki/Error:-Catch-filter-must-inherit-from-Error-or-be-a-simple-predicate-function instead.

If this catch is here to catch errors of imagefs.readFile(configFilePath), can we put it right after it (before JSON.parse)?

Done.

zvin · 2017-10-30T10:55:16Z

lib/network.coffee

+	.then ->
+		schema = getOS2WifiConfigurationSchema(manifest, answers)
+		if manifest.configuration.config.image
+			image = path.join(image, manifest.configuration.config.image)


I must be missing something but how does this work if image is not a folder ?

Anwswer to myself: that must be for edison images.

Exactly. If you try to configure a single image as an edison image you'll get errors complaining that myimage.img/resin-image-edison.hddimg does not exist, but imo that's not an unreasonable error, and it's a pretty rare case (and trying to configure a raspberry pi image to work for an edison is always going to blow up eventually).

zvin · 2017-10-30T10:56:19Z

lib/network.coffee

+	###
+	connectionsFolderDefinition = utils.definitionForImage(target, getConfigPathDefinition(manifest, CONNECTIONS_FOLDER))
+
+	imagefs.listDirectory(connectionsFolderDefinition).then (files) ->


NIT: can we put the .then (files) -> on the next line?

zvin · 2017-10-30T11:04:10Z

lib/network.coffee

+		}
+		{
+			domain: [
+				[ 'network_config', 'service_home_ethernet' ],


NIT: no comma needed here.

zvin · 2017-10-30T11:04:16Z

lib/network.coffee

+			location:
+				getConfigPathDefinition(manifest, '/config.json', false)
+		network_config:
+			type: 'ini',


NIT: no comma needed here.

zvin · 2017-10-30T11:04:20Z

lib/network.coffee

+		network_config:
+			type: 'ini',
+			location:
+				parent: 'config_json',


NIT: no comma needed here.

zvin · 2017-10-30T11:04:24Z

lib/network.coffee

+	mapper: [
+		{
+			domain: [
+				[ 'config_json', 'wifiSsid' ],


NIT: no comma needed here.

zvin · 2017-10-30T11:12:52Z

lib/utils.coffee

 rindle = Promise.promisifyAll(require('rindle'))
 path = require('path')
 stringToStream = require('string-to-stream')
+streamToString = require('stream-to-string')


Why use streamToString instead of rindle.extractAsync ?

Because I didn't know it existed 😃. Fixed.

pimterry · 2017-10-30T21:59:47Z

Updated to fix all the comments, except a couple I've replied to separately, and I've also added proper tests for network configuration.

The tests are failing on travis because our ext2fs module uses const, and doesn't actually support Node 4. We don't support Node 4 in the CLI either though, so we could just drop those tests drop support completely here too, unless there's some other reason that needs this to work in Node 4? We could probably make ext2fs backward compatible if we need to, but if not it'd be nice to keep it modern.

Meanwhile, the tests are failing on appveyor because ext2fs doesn't build on windows. This is going to be a bigger problem, since it really needs to build reliably on windows with nothing more than windows-build-tools, or none of our windows users are going to successfully install it. Right now, with that package installed, the current error is:

C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V120\Microsoft.Cpp.Platform.targets(64,5):
error MSB8020: The build tools for v140 (Platform Toolset = 'v140') cannot be found. To
build using the v140 build tools, please install v140 build tools. Alternatively, you may
upgrade to the current Visual Studio tools by selecting the Project menu or right-click
the solution, and then selecting "Upgrade Solution...".
[C:\projects\resin-device-init-xpd6l\node_modules\ext2fs\build\ext2fs.vcxproj]

I'm not immediately sure how to fix that - any ideas? I can dig into it more, but some pointers in the right direction first would be really useful.

pimterry · 2017-11-22T12:41:28Z

Node-ext2fs issues seem to now be resolved. Next issue that's currently breaking the tests is balena-io/reconfix#33: the reconfix JS implementation breaks with directories on windows. Quick fix, hopefully this will start passing once that's sorted & released.

Change-Type: patch

Connects-To: balena-io/balena-cli#494 Change-Type: minor

…nfig.json

Change-Type: patch

pimterry · 2017-11-22T16:36:30Z

It's alive! Seems to all be working happily on Windows now. I think this means this PR is finally all good and ready to go.

@zvin @abrodersen can you review please?

zvin · 2017-11-22T17:24:42Z

lib/utils.coffee

 		image: image
-		partition:
-			primary: 1
+		partition: 1


This won't work on a jetson-tx2 where boot is the 12th partition.
Shouldn't matter as there is a fallback.

zvin · 2017-11-22T17:27:28Z

lib/utils.coffee

+exports.getImageOsVersion = (image) ->
+	Promise.using imagefs.read(
+		image: image
+		partition: 2


This won't work on a jetson where resin-rootA is the 13th partition and uses the new host apps tree.

Yep, this doesn't work for quite a few similar cases too, so there's a .catchReturn(null) below, which should work as the fallback. Returning null from here results in us configuring the image as both OS1 and OS2 for any images where we can't get a specific version easily. That's a little messy, but acceptable for now, and shouldn't break anything.

The plan in future is to eventually stop needing to look at the root partition at all this - instead we'll have an os-release file inside the boot partition itself, so we can use the device types to find it as we do with config.json/system-connections/etc. @agherzan is looking at that here: balena-os/meta-balena#906.

zvin · 2017-11-22T17:30:43Z

tests/e2e.coffee

+		Promise.using imagefs.read(
+			path: '/config.json'
+			image: path.join(images.edison, 'resin-image-edison.hddimg')
+		), extract


Extract could be replaced with rindle.extractAsync.

True, good idea, but I'm going to ignore this for now anyway. I want to do a bunch of improvements to the tests (e.g. #26) anyway, and we use this extract method everywhere, so I'll probably replace it all with rindle as part of that.

Should've done this originally really. It'd be nice to also fix the underlying streaming bug (which segfaults when trying to stream a missing file through ext2fs), but this fixes the issue, and we'll want this change regardless.

resin-io-modules-versionbot · 2017-11-28T18:29:30Z

VersionBot failed to carry out a status check for the above pull request here: #25. The reason for this is:
Reference already exists
Please carry out relevant changes or alert an appropriate admin.

pimterry requested a review from zvin October 27, 2017 12:27

pimterry mentioned this pull request Oct 27, 2017

Test with real images #26

Open

zvin reviewed Oct 30, 2017

View reviewed changes

pimterry force-pushed the configure-os2-network branch from f78d376 to 150e0cf Compare October 31, 2017 09:28

pimterry mentioned this pull request Nov 3, 2017

Publish windows builds with node-pre-gyp-github automatically balena-io-modules/node-ext2fs#9

Closed

pimterry mentioned this pull request Nov 16, 2017

Set up automated pre-gyp publishing for ext2fs balena-io-modules/node-ext2fs#11

Merged

pimterry force-pushed the configure-os2-network branch 2 times, most recently from 9cc258b to 493ab39 Compare November 21, 2017 17:58

pimterry mentioned this pull request Nov 22, 2017

Fix issues with paths when using windows balena-io/reconfix#33

Merged

pimterry added 14 commits November 22, 2017 15:59

Upgrade to resin-image-fs@4, adding ext support

a38cef8

Change-Type: patch

Automatically build docs when you build and build before you test

bd1c661

Add an ext4 partition with valid /etc/os-release files to test images

5d8d710

Read os-release from device images to get their OS version

af283cc

Make .configure() configure network settings for both 1.x & 2.x

afc1ec0

Connects-To: balena-io/balena-cli#494 Change-Type: minor

Handle device-types with no yocto info included

405abab

Ensure we can correctly configure edison images and images with no co…

fb8a3c2

…nfig.json

Post-review style tweaks

2fdafe2

Use rindle instead of stream-to-string

9a57f58

Make sure we wait for OS2 network configuration to complete

113907d

Add tests for network configuration

991734e

Update to new resin-image-fs, with prebuild node-ext2fs

24ed40c

Test with node 8, stop testing node 4

e903399

Upgrade to reconfix 0.0.4 to fix windows support

b06c3b3

Change-Type: patch

pimterry force-pushed the configure-os2-network branch from 914400c to b06c3b3 Compare November 22, 2017 16:22

pimterry requested review from abrodersen and zvin November 22, 2017 16:35

zvin approved these changes Nov 22, 2017

View reviewed changes

pimterry added the procbots/versionbot/ready-to-merge label Nov 28, 2017

v4.0.1

65b17f7

resin-io-modules-versionbot bot merged commit d1858cb into master Nov 28, 2017

This was referenced Nov 28, 2017

Versionbot: VB does bizarre things if the PR has a prerelease version bump balena-io-modules/balena-procbots#348

Open

Fix versionbot's changelog mess #28

Merged

pimterry deleted the configure-os2-network branch November 29, 2017 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make .configure() configure network settings for both 1.x & 2.x #25

Make .configure() configure network settings for both 1.x & 2.x #25

pimterry commented Oct 27, 2017 •

edited

Loading

zvin Oct 30, 2017

zvin Oct 30, 2017

pimterry Oct 30, 2017

zvin Oct 30, 2017

zvin Oct 30, 2017

zvin Oct 30, 2017

pimterry Oct 30, 2017

zvin Oct 30, 2017

zvin Oct 30, 2017

pimterry Oct 30, 2017

zvin Oct 30, 2017

zvin Oct 30, 2017

zvin Oct 30, 2017

zvin Oct 30, 2017

zvin Oct 30, 2017

zvin Oct 30, 2017

pimterry Oct 30, 2017

pimterry commented Oct 30, 2017

pimterry commented Nov 22, 2017

pimterry commented Nov 22, 2017

zvin Nov 22, 2017

zvin Nov 22, 2017

pimterry Nov 22, 2017

zvin Nov 22, 2017

pimterry Nov 28, 2017

resin-io-modules-versionbot bot commented Nov 28, 2017

Make .configure() configure network settings for both 1.x & 2.x #25

Make .configure() configure network settings for both 1.x & 2.x #25

Conversation

pimterry commented Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pimterry commented Oct 30, 2017

pimterry commented Nov 22, 2017

pimterry commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

resin-io-modules-versionbot bot commented Nov 28, 2017

pimterry commented Oct 27, 2017 •

edited

Loading