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

Upgrade to new resin-request and resin-register-device #245

Merged
merged 2 commits into from
Jan 6, 2017

Conversation

pimterry
Copy link
Contributor

As with balena-io-modules/balena-register-device#28, this is just a work in progress placeholder until balena-io-modules/balena-request#82 is ready, since I'm off for Christmas imminently.

Once the resin-request and resin-register-device PRs are merged and released this needs updating to depend on those directly, and then it's good to go, I think.

@pimterry
Copy link
Contributor Author

Bonus 🎁: this branch didn't build before the PR, and has only built once for each CI service on this PR, which means #241 and #244 are all done too.

@pimterry
Copy link
Contributor Author

Strange - the build fails, because resin-pine is still independently using the old version of resin-request, which expects a global fetch (and so does also need fixing really), but a clean npm install && npm test for me locally shares the one (working) resin-request instance and is fine.

I'm not going to try quickly patching resin-pine, which I've never looked at before, right before I run out the door. Should be fairly easy to patch up in line with this change though - I'll finish this change up there when I'm back, or feel free to duplicate this PR over there yourself instead if you need this merged sooner.

@Page-
Copy link
Contributor

Page- commented Dec 20, 2016

Could it be the yarn shrinkwrapping feature keeping it on the old version?

@@ -61,8 +60,8 @@
"resin-device-status": "^1.0.1",
"resin-errors": "^2.4.0",
"resin-pine": "^4.0.0",
"resin-register-device": "^3.0.0",
"resin-request": "^6.2.0",
"resin-register-device": "https://github.com/resin-io-modules/resin-register-device#migrate-to-resin-request",
Copy link
Contributor

Choose a reason for hiding this comment

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

Give me a ping once these are back on semver and I'll approve

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping @Page-

@emirotin
Copy link
Contributor

emirotin commented Jan 3, 2017

So was the issue with resin-pine cleared by the corresponding PR?

@pimterry
Copy link
Contributor Author

pimterry commented Jan 3, 2017

It will be. I've got some Landr related things to do right now, but I'm going to finish that PR and resin-register-device early this afternoon, do some releases, and then I think this PR should magically become mergeable.

@emirotin
Copy link
Contributor

emirotin commented Jan 3, 2017

Good. As soon as you have those done I will also bump the outdated dependencies in all our modules

@pimterry pimterry changed the title [WIP] Upgrade to new resin-request and resin-register-device Upgrade to new resin-request and resin-register-device Jan 5, 2017
This now uses the new injection dependency approach, so we build each
of these libraries and glue them together, rather than them internally
instantiating their own local versions of one another.
@pimterry
Copy link
Contributor Author

pimterry commented Jan 5, 2017

@emirotin All 3 modules are now updated and released, and I've just an update to this PR pulling all that in, and correctly doing all the DI setup. I think once this builds and is approved it's good to go in, and we'll have up to date versions throughout.

@emirotin
Copy link
Contributor

emirotin commented Jan 5, 2017

  1) SDK Integration Tests given a logged in fresh user given a single application with two offline devices that share the same uuid root Device Model resin.models.device.has() should be rejected with an error for an ambiguous shorter uuid:
     AssertionError: expected promise to be rejected with 'ResinAmbiguousDevice' but it was fulfilled with true

Hm. Race conditions?
Restarted the job.

@emirotin
Copy link
Contributor

emirotin commented Jan 5, 2017

And now

     ResinDeviceNotFound: Device not found: b4ab9d37052c0b4e1f03cc6a1ced081711f8a17a8915c9073b74010fa948c2

Some schroedinbugs. I suggest we merge this PR (it's very unlikely the issues are related) and think about proper randomized fixtures while finalizing the browser-sdk branch.

@emirotin
Copy link
Contributor

emirotin commented Jan 5, 2017

BTW now that we have yarnfile when changing the dependencies we need to regenerate it: simply run yarn upgrade

@emirotin
Copy link
Contributor

emirotin commented Jan 5, 2017

Made the tests pass by rerunning them couple of times.

@pimterry
Copy link
Contributor Author

pimterry commented Jan 6, 2017

@emirotin Good point on Yarn, I've pushed the upgrade.

This is one thing that would be partially fixed if we start using Yarn in CI. If Travis used Yarn then we would've got loud obvious build failures for that. Doesn't catch all upgrades, but anything certainly required to pass the tests.

@emirotin
Copy link
Contributor

emirotin commented Jan 6, 2017

Travis does use yarn automatically if it detects the yarn.lock file. So I was surprised why the tests didn't fail on missing dependencies.

@pimterry
Copy link
Contributor Author

pimterry commented Jan 6, 2017

What? You're totally right (https://travis-ci.org/resin-io/resin-sdk/jobs/189204990 did indeed use Yarn, and not npm), but that means that build definitely shouldn't have succeeded. Old Resin-Request has a different factory that should have meant it didn't get initialised here, and then it should've exploded when isomorphic fetch wasn't required anyway. That makes zero sense.

I'm going to merge this anyway, since the build does (mostly) pass and it's clearly working properly locally, but this is confusing, and a bit concerning.

@pimterry pimterry merged commit 0e12039 into sdk-browser Jan 6, 2017
@pimterry pimterry deleted the update-resin-request branch January 6, 2017 10:02
@pimterry pimterry mentioned this pull request Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants