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

feat: add applyDefaultOnWrites property [3.x] #1770

Merged
merged 1 commit into from
Aug 19, 2019
Merged

feat: add applyDefaultOnWrites property [3.x] #1770

merged 1 commit into from
Aug 19, 2019

Conversation

hacksparrow
Copy link
Contributor

Description

Adds the ability to ignore writing default values to the database.

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

raymondfeng
raymondfeng reviewed Aug 9, 2019
View reviewed changes
lib/dao.js Outdated Show resolved Hide resolved
raymondfeng
raymondfeng reviewed Aug 9, 2019
View reviewed changes
lib/dao.js Show resolved Hide resolved
@hacksparrow hacksparrow force-pushed the 3x/defaultValueHandling branch 5 times, most recently from 58831cf to ca2ce28 Compare August 9, 2019 15:42
@hacksparrow
Copy link
Contributor Author

@raymondfeng I have applied your suggestions. PTAL.

@hacksparrow hacksparrow changed the title feat: add defaultValueHandling property feat: add applyDefaultOnWrites property Aug 14, 2019
jannyHou
jannyHou approved these changes Aug 14, 2019
View reviewed changes
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

👍 The solution is reasonable to me.

bajtos
bajtos reviewed Aug 15, 2019
View reviewed changes
Copy link
Member

@bajtos bajtos 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 in general, the tests can be improved a bit.

test/defaults.test.js Outdated Show resolved Hide resolved
test/defaults.test.js Outdated Show resolved Hide resolved
test/defaults.test.js Outdated Show resolved Hide resolved
test/defaults.test.js Outdated Show resolved Hide resolved
@hacksparrow hacksparrow force-pushed the 3x/defaultValueHandling branch 2 times, most recently from fe99fd7 to 256952d Compare August 16, 2019 10:33
@hacksparrow
Copy link
Contributor Author

Thanks for the feedback @bajtos. Applied all of them.

What do we do about the downstream errors?

@dhmlau
Copy link
Member

dhmlau commented Aug 16, 2019

Not sure about loopback. For loopback-connector-oracle, we have a ticket to track that: loopbackio/loopback-connector-oracle#183.

@bajtos
Copy link
Member

bajtos commented Aug 19, 2019

What do we do about the downstream errors?

The loopback build failed because a network error while downloading Node and/or dependencies.

06:56:20 Now using node v8.16.1 (npm v6.4.1)
06:56:20 Installing default global packages from /home/jenkins/.nvm/default-packages...
06:57:30 npm ERR! code ECONNREFUSED
06:57:30 npm ERR! errno ECONNREFUSED
06:57:30 npm ERR! FetchError: request to http://10.91.203.23:48584/semver failed, reason: connect ECONNREFUSED 10.91.203.23:48584

Can you try to re-run the tests?

bajtos
bajtos approved these changes Aug 19, 2019
View reviewed changes
test/defaults.test.js Outdated Show resolved Hide resolved
feat: add applyDefaultOnWrites property
020d317 
Adds the ability to ignore writing default values to the database.
@hacksparrow hacksparrow force-pushed the 3x/defaultValueHandling branch from 256952d to 020d317 Compare August 19, 2019 08:40
@hacksparrow hacksparrow mentioned this pull request Aug 19, 2019
Merged
2 tasks
@hacksparrow hacksparrow self-assigned this Aug 19, 2019
@bajtos bajtos changed the title feat: add applyDefaultOnWrites property feat: add applyDefaultOnWrites property [3.x] Aug 19, 2019
@dhmlau
Copy link
Member

dhmlau commented Aug 19, 2019

@slnode test please

@hacksparrow hacksparrow merged commit 020d317 into 3.x Aug 19, 2019
@delete-merged-branch delete-merged-branch bot deleted the 3x/defaultValueHandling branch August 19, 2019 15:29
@hacksparrow hacksparrow mentioned this pull request Aug 19, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jannyHou jannyHou jannyHou approved these changes

@bajtos bajtos bajtos approved these changes

@raymondfeng raymondfeng raymondfeng approved these changes

@b-admike b-admike Awaiting requested review from b-admike

@dhmlau dhmlau Awaiting requested review from dhmlau dhmlau is a code owner

@zbarbuto zbarbuto Awaiting requested review from zbarbuto

Assignees

@hacksparrow hacksparrow

Labels
customer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hacksparrow @dhmlau @bajtos @raymondfeng @jannyHou