Skip to content

Conversation

@keedya
Copy link

@keedya keedya commented Jul 9, 2016

  • Able to switch on / by default in one place

@keedya
Copy link
Author

keedya commented Jul 9, 2016

@zyoung51 @brianparry

@keedya keedya closed this Jul 9, 2016
@keedya keedya reopened this Jul 9, 2016
);

//re-route common and current
var varsionPath = configuration.get('versionBase') || '1.1';
Copy link
Contributor

Choose a reason for hiding this comment

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

versionPath instead of varsionPath?

Copy link
Author

Choose a reason for hiding this comment

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

Oh wow, totally missed that!! Thanks @zyoung51

@zyoung51
Copy link
Contributor

zyoung51 commented Jul 9, 2016

Just a nitpick on the variable name. LGTM, +1

- Able to switch on / by default in one place
@keedya
Copy link
Author

keedya commented Jul 10, 2016

@zyoung51 all set

@yyscamper
Copy link
Contributor

👍

);

//re-route common and current
var versionPath = configuration.get('versionBase') || '1.1';
Copy link
Contributor

Choose a reason for hiding this comment

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

@keedya I think we should add an assert or some other type of check to verify that versionBase is set to a valid value.

@brianparry
Copy link
Contributor

👍 One minor comment related to config validation, but I think that's more of a general problem outside the scope of this PR.

@brianparry
Copy link
Contributor

test this please

@brianparry brianparry merged commit 6b5fe6c into RackHD:master Jul 11, 2016
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