Skip to content

Node Tagging: #88

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
Merged
jlongever merged 1 commit into from
Feb 23, 2016
Merged

Node Tagging: #88

jlongever merged 1 commit into from
Feb 23, 2016

Conversation

@zyoung51
Copy link
Contributor

@zyoung51 zyoung51 commented Feb 16, 2016

  • Modify node model to have a tags array and add $addToSet and $pull native
    mongo commands to modify the array
  • Create a tags model with SKU model based rules

houndci-bot
houndci-bot reviewed Feb 16, 2016
View reviewed changes
lib/models/tags.js Outdated
'Promise'
];

function TagModelFactory (Model, _, assert, Validatable, Anchor, Promise) {
Copy link

@houndci-bot houndci-bot Feb 16, 2016

Choose a reason for hiding this comment

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

'Promise' is defined but never used.

Copy link
Member

@heckj heckj Feb 18, 2016

Choose a reason for hiding this comment

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

planning on using Promise later in here, or was this a "just in case" that didn't get used?

Copy link
Contributor Author

@zyoung51 zyoung51 Feb 18, 2016

Choose a reason for hiding this comment

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

I'll take it out. I can add it back in later if/when I need it.

@zyoung51
Copy link
Contributor Author

zyoung51 commented Feb 16, 2016

@RackHD/corecommitters @uppalk1 @keedya @tannoa2

@JenkinsRHD
Copy link
Contributor

JenkinsRHD commented Feb 16, 2016 

*** BUILD #233 ***

@jlongever
Copy link
Contributor

jlongever commented Feb 16, 2016

test this please

@zyoung51 zyoung51 mentioned this pull request Feb 17, 2016
Merged
@zyoung51
Copy link
Contributor Author

zyoung51 commented Feb 17, 2016

test this please

@JenkinsRHD
Copy link
Contributor

JenkinsRHD commented Feb 17, 2016 

*** BUILD #236 ***
Test Name: Model multiple records Shuld destroy by criteria
Error Details: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.
Stack Trace: Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.

@zyoung51 zyoung51 mentioned this pull request Feb 17, 2016
Merged
heckj
heckj reviewed Feb 17, 2016
View reviewed changes
spec/lib/models/node-spec.js
};

before(function() {
sinon.stub(this.model, "native", function(cb) { cb(null, collection); });
Copy link
Member

@heckj heckj Feb 17, 2016

Choose a reason for hiding this comment

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

teachme question - why stub out model.native like this? Not cluing in how it's used by the tests.... (not clear on what model.native is - are we just using it to make sure the collections got called and invoked?)

Copy link
Contributor Author

@zyoung51 zyoung51 Feb 18, 2016

Choose a reason for hiding this comment

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

It is stubbed out with this pattern so the underlying code can promisify the function. It is being stubbed out to remove the mongodb dependency from the unittest. Functional/integration testing would be used to validate MongoDB behavior/interactions, not unittests.

Copy link
Member

@heckj heckj Feb 18, 2016

Choose a reason for hiding this comment

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

No qualms with the tactics, just trying to grok where .native normally resides. I'm not clear on where it's getting promisified. Is native something that's already in the waterline model objects and is a convenient hook for capturing the flow that would otherwise go down into the MongoDB drivers?

@zyoung51 zyoung51 force-pushed the feature/add-tagging branch from b1664f0 to d6bf667 Compare February 18, 2016 13:41
@zyoung51 zyoung51 mentioned this pull request Feb 18, 2016
Closed
@anhou
Copy link
Member

anhou commented Feb 18, 2016

👍

@jlongever
Copy link
Contributor

jlongever commented Feb 19, 2016

test this please

1 similar comment
@jlongever
Copy link
Contributor

jlongever commented Feb 19, 2016

test this please

Node Tagging:
4207256 
- Modify node model to have a tags array and add $addToSet and $pull native
  mongo commands to modify the array
- Create a tags model with SKU model based rules
@zyoung51 zyoung51 force-pushed the feature/add-tagging branch from d6bf667 to 4207256 Compare February 19, 2016 16:25
@zyoung51
Copy link
Contributor Author

zyoung51 commented Feb 22, 2016

test this please

@JenkinsRHD
Copy link
Contributor

JenkinsRHD commented Feb 22, 2016 

*** BUILD #247 ***

@jlongever
Copy link
Contributor

jlongever commented Feb 22, 2016

test this please

@JenkinsRHD
Copy link
Contributor

JenkinsRHD commented Feb 22, 2016 

*** BUILD #248 ***

@jlongever
Copy link
Contributor

jlongever commented Feb 22, 2016

@zyoung51 looks like on-taskgraph is failing unit-tests with on-core applied to it. Would you mind checking it out locally?

@zyoung51
Copy link
Contributor Author

zyoung51 commented Feb 22, 2016

@jlongever It is passing on-taskgraph master w. on-core add-tagging locally.

@jlongever
Copy link
Contributor

jlongever commented Feb 22, 2016

test this please

@JenkinsRHD
Copy link
Contributor

JenkinsRHD commented Feb 22, 2016 

*** BUILD #249 ***

@zyoung51
Copy link
Contributor Author

zyoung51 commented Feb 22, 2016

test this please

@JenkinsRHD
Copy link
Contributor

JenkinsRHD commented Feb 22, 2016 

*** BUILD #251 ***

@zyoung51
Copy link
Contributor Author

zyoung51 commented Feb 23, 2016

test this please

@jlongever
Copy link
Contributor

jlongever commented Feb 23, 2016

test this please

@jlongever
Copy link
Contributor

jlongever commented Feb 23, 2016

+1

jlongever added a commit that referenced this pull request Feb 23, 2016
@jlongever
Merge pull request #88 from zyoung51/feature/add-tagging
41819d6 
Node Tagging:
@jlongever jlongever merged commit 41819d6 into RackHD:master Feb 23, 2016
@keedya
Copy link

keedya commented Feb 24, 2016

👍

@zyoung51 zyoung51 deleted the feature/add-tagging branch February 24, 2016 15:41
dalebremner pushed a commit to dalebremner/on-core that referenced this pull request Dec 1, 2017
@jlongever
Merge pull request RackHD#88 from jlongever/example/add_config_variables
ee0b1a7 
Add vagrant example configuration variables.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@zyoung51 @JenkinsRHD @jlongever @anhou @keedya @heckj @benbp @houndci-bot