Skip to content

Refactor fetch instance resource from API for error message generation#8

Merged
Nathan219 merged 44 commits intomasterfrom
SAN-3006-refactor-detention
Dec 5, 2015
Merged

Refactor fetch instance resource from API for error message generation#8
Nathan219 merged 44 commits intomasterfrom
SAN-3006-refactor-detention

Conversation

@cflynn07
Copy link
Copy Markdown
Contributor

Refactor to fetch instance object directly from API for error display information generation.
Co-PR: CodeNow/navi#94

Review:

@cflynn07 cflynn07 changed the title Refactor detention Refactor fetch instance resource from API for error message generation Nov 28, 2015
Comment thread bin/www 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.

Let's use Bunyan here. Also error cat create and report error and exit on cb

Comment thread bin/www
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.

move this above app.login, was a bug before, if listen error sync it will not get called.

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.

also where is onError ?!

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.

ahh nm, git diff messed me up

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Was confused by what you thought you were seeing....

Comment thread logger.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.

nothing here?

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.

remove unless you plan on adding something in the near future

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I left it because yeah, I imagine it wont be long at all until I add something.

Comment thread app.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if ports is undefined here, it causes an exception

Comment thread app.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use req.instance.getBranchName() here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done
4c6656c

Comment thread app.js
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is null, which makes the links (view logs, go to container) not work

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Did you see this? #8 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's weird that the comment wasn't on the full change...

@Nathan219
Copy link
Copy Markdown
Member

LGTM

Nathan219 added a commit that referenced this pull request Dec 5, 2015
Refactor fetch instance resource from API for error message generation
@Nathan219 Nathan219 merged commit 408f594 into master Dec 5, 2015
@Nathan219 Nathan219 deleted the SAN-3006-refactor-detention branch December 5, 2015 01:30
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.

4 participants