Skip to content

Add more logging in the autofork route#1251

Merged
bkendall merged 3 commits intomasterfrom
SAN-3170-logging
Jan 11, 2016
Merged

Add more logging in the autofork route#1251
bkendall merged 3 commits intomasterfrom
SAN-3170-logging

Conversation

@Nathan219
Copy link
Copy Markdown
Member

There isn't much here to log errors! We need some

Comment thread lib/models/apis/runnable.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

forkMasterInstance -> Runnable.prototype.forkMasterInstance

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You and Casey seem to have a difference in opinion:
#1192 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have no rebuttal for that...

@bkendall
Copy link
Copy Markdown
Contributor

bkendall commented Jan 4, 2016

oops, don'

pretty solid commit message, that. 😉

@bkendall bkendall added the review label Jan 8, 2016
@bkendall bkendall self-assigned this Jan 8, 2016
Comment thread lib/models/apis/runnable.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

make this logData. define it above.

Comment thread lib/models/apis/runnable.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this normally goes at the top of the function: line 105

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

well, since body isn't defined until well into the function, it's not going to be moved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

well tx: true is defined at the top. In the places we use logData, I have only seen it defined at the top of the function. consistency is good ;)

bkendall added a commit that referenced this pull request Jan 11, 2016
Add more logging in the autofork route
@bkendall bkendall merged commit 903a6e1 into master Jan 11, 2016
@bkendall bkendall deleted the SAN-3170-logging branch January 11, 2016 20:00
@bkendall bkendall removed their assignment Jul 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants