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.

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

Lldp lshw upload status #123

Merged
merged 7 commits into from
Mar 4, 2014
Merged

Conversation

byxorna
Copy link
Contributor

@byxorna byxorna commented Feb 21, 2014

Allows LLDP/LSHW updates to assets in configurable statuses, instead of disallowing updates when in status other than [NEW,INCOMPLETE,MAINTENANCE].

i.e.

features{
  allowedServerUpdateStatuses = [ PROVISIONED, PROVISIONING ]
}

@byxorna
Copy link
Contributor Author

byxorna commented Feb 21, 2014

@dallasmarlow @Primer42

@@ -89,9 +89,11 @@ object AssetLifecycle {
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Call me crazy - is this just a cosmetic change? Did you actually change any functionality here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No functionality change. Just did it out of OCD. I can revert this change if you would like, as it is not necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I totally understand OCD / having consistent, clearer code. I just wanted to make sure I wasn't missing a subtle diff. Leave it.

@william-richard
Copy link
Contributor

LGTM

Asset.inTransaction {
options.get("lshw").foreach{lshw =>
parseLshw(asset, new LshwParser(lshw)).left.foreach{throw _}
InternalTattler.informational(asset, None, "Parsing and storing LSHW data succeeded")
Copy link
Contributor

Choose a reason for hiding this comment

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

nice use of the tattler

@byxorna
Copy link
Contributor Author

byxorna commented Feb 28, 2014

@dallasmarlow @Primer42 addressed your concerns, and confirmed this is functional.

@@ -155,6 +149,31 @@ object AssetLifecycle {
}.left.map(e => handleException(asset, "Error updating status/state for asset", e))
}

protected def updateAnyServer(asset: Asset, options: Map[String,String]): Status[Boolean] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

@byxorna totally a nitpick, but i think this method name is a bit misleading. you can't actually update "any" server, just server assets that are in a configured allowed status for updates. why not just updateServer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because there is already an updateServer with the same signature :(
https://github.com/tumblr/collins/pull/123/files#diff-0d4ee0e3fc6b2506b41bee0e38c66d21L101

@byxorna
Copy link
Contributor Author

byxorna commented Feb 28, 2014

@dallasmarlow changed updateAnyServer to updateServerHardwareMeta.

@dallasmarlow
Copy link
Contributor

@byxorna lgtm, lets deploy tomorrow

dallasmarlow added a commit that referenced this pull request Mar 4, 2014
@dallasmarlow dallasmarlow merged commit b6f1f95 into tumblr:master Mar 4, 2014
byxorna added a commit to byxorna/collins that referenced this pull request Mar 5, 2014
william-richard pushed a commit that referenced this pull request Mar 7, 2014
add documentation for feature added in #123
dalehamel pushed a commit to Shopify/collins that referenced this pull request Jun 10, 2014
dalehamel pushed a commit to Shopify/collins that referenced this pull request Jun 10, 2014
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.

3 participants