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.

Sign up for GitHub

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

Jump to bottom

Factory function defaults #260

Merged
merged 1 commit into from
Jan 26, 2017
Merged

Factory function defaults #260

merged 1 commit into from
Jan 26, 2017

Conversation

craigmulligan
Copy link
Contributor

connects to #258 and some of #251.

Defaults are as follows:

apiUrl: 'https://api.resin.io/'
apiVersion: 'v2',
isBrowser: window?,
imageMakerUrl: 'https://img.resin.io/',
dataDirectory: '/opt/local/resin'

Not sure about dataDirectory as it requires root privileges?

lurch
lurch reviewed Jan 16, 2017
View reviewed changes
tests/integration.spec.coffee

it 'should return an object with valid keys', ->
mockResin = getSdk()
m.chai.expect(mockResin).to.include.keys(validKeys)
Copy link

Choose a reason for hiding this comment

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

Should the default values also be unit-tested?

Copy link
Contributor

Choose a reason for hiding this comment

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

The code for them is so trivial it's not strictly required. If that means exposing the object (to make it reachable from the test code) I'd rather not make this change.

@craigmulligan
Copy link
Contributor Author

craigmulligan commented Jan 16, 2017 •
edited
Loading

Ah I see, resin-settings-client already has defaults. So better to get it working in the browser right..

@emirotin
Copy link
Contributor

resin-settings-client will not work in the browser, it's fs-only solution. It has no analogs for the browser because the use-case is fundamentally different.

With the CLI you have the workplace (your fs) that's persisted between the CLI runs.
In the UI we have the localStorage used to persist the token, but other than that the params (like the API host) are provided at run-time. You don't want to save the API host in the localStorage at the first run and the re-read it after that.

@craigmulligan
Copy link
Contributor Author

Ah I see, so I've explicitly set the browser defaults and then fetch the settings from the settings.client if in a node environment. Sound reasonable?

@craigmulligan
Copy link
Contributor Author

hmm browser tests are failing on Travis but are passing locally?

@craigmulligan
Copy link
Contributor Author

@lekkas any idea why the test would fail on travis but not locally? You ever had that?

@craigmulligan
Copy link
Contributor Author

@emirotin @pimterry I see both of you ran into the same test failures on previous builds (here + here)? the failures don't look to have anything to do with this PR so I'm wondering if there is some condition that's causing the failure? Do you have any ideas?

@pimterry
Copy link
Contributor

Hey @craig-mulligan sorry for the delay I've been away. This looks like it's being caused by #252.

If you can't reproduce the issue locally, and you're not seeing exactly the same failure between the two test runs, I think you can assume that it's really being caused by that intermittent issue. If you're not sure you can rerun the test job a few times, and it should get to a passing state fairly soon. It definitely looks like the same spurious failure to me though.

We should definitely fix this, I'll try and get some time to take a look at it properly later this week.

@craigmulligan
Copy link
Contributor Author

@pimterry ah, cheers, thanks for pointing that out. I ran it again. It worked on node 4 but no 6 so must be an intermittent issue.

Give me a shout if there is anything I can do to help with the testing when you get to it. :)

emirotin
emirotin reviewed Jan 24, 2017
View reviewed changes
lib/resin.coffee

if opts.isBrowser
settings =
get: notImplemented
getAll: notImplemented
else
settings = require('resin-settings-client')
defaults opts,
settings.getAll()
Copy link
Contributor

Choose a reason for hiding this comment

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

One reservation I have is that here we're lazy and hope that the settings client uses the exact same names for the properties that are used by the SDK. While this assumption sounds reasonable it's not ensured by any contract.
I'd also rather use the explicit whitelist instead of blindly passing everything stored in the settings (that are general-purposed and can be used by the user's code).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, so explicitly extract them eg:dataDirectory: settings.get('dataDirectory') ?

@emirotin
Copy link
Contributor

emirotin commented Jan 26, 2017 via email

@craigmulligan
Copy link
Contributor Author

Great, fixed!

emirotin
emirotin reviewed Jan 26, 2017
View reviewed changes
lib/resin.coffee
apiVersion: 'v2',
isBrowser: window?
isBrowser: window?,
Copy link
Contributor

Choose a reason for hiding this comment

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

Unneeded comma?

emirotin
emirotin reviewed Jan 26, 2017
View reviewed changes
lib/resin.coffee

if opts.isBrowser
settings =
get: notImplemented
getAll: notImplemented
else
settings = require('resin-settings-client')
defaults opts,
imageMakerUrl: settings.get('imageMakerUrl'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Unneeded comma too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fkn coffee script :)

emirotin
emirotin approved these changes Jan 26, 2017
View reviewed changes
Copy link
Contributor

@emirotin emirotin left a comment

Choose a reason for hiding this comment

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

Please squash before merging

@emirotin
Copy link
Contributor

Oh, and can we have the changelog entry please?

@craigmulligan
Copy link
Contributor Author

Will do, we should add versionist, I'll create an issue.

Add defaults for single factory function
16eecb6 
This makes all options truly optional eg. you can now - require('resin-sdk')();

Change-type: Patch
@craigmulligan craigmulligan merged commit a2beb89 into master Jan 26, 2017
@pimterry pimterry deleted the factory-function-defaults branch April 13, 2017 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lurch lurch lurch left review comments

@emirotin emirotin emirotin approved these changes

@jviotti jviotti Awaiting requested review from jviotti

@lekkas lekkas Awaiting requested review from lekkas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@craigmulligan @emirotin @pimterry @lurch