tumblr / collins

groovy kind of love
tumblr.github.com/collins
Apache License 2.0
571 stars 99 forks source link

Return NODECLASS in API #482

Closed discordianfish closed 7 years ago

discordianfish commented 7 years ago

Hi,

the API doesn't return the nodeclass right now but I want to use that to determine different partition schemes in my installer. I think this isn't a uncommon use case. What do you think?

discordianfish commented 7 years ago

This is btw the output for tumblrtag1 after adding a empty classifier:

    },
    "NODECLASS": {
      "ID": 2,
      "TAG": "app-classifier",
      "STATE": null,
      "STATUS": "Allocated",
      "TYPE": "CONFIGURATION",
      "CREATED": "2016-10-28T19:29:43",
      "UPDATED": "2016-10-28T19:30:53",
      "DELETED": null
    },
    "HARDWARE": {

I personally only need the tag, so could also use that instead if you prefer. I'll also submit a PR to update the go bindings if we agree on adding this.

byxorna commented 7 years ago

@discordianfish I think calling it something more specific than NODECLASS may help reduce confusion. In earlier work, I tried to move naming this feature to "asset classification" to explicitly differentiate it from the NODECLASS attribute that collins uses for your provisioning profile.

I like this though! Ive wanted this feature for ages, never thought it would be this easy to add it :) (cc @alex-laties)

discordianfish commented 7 years ago

I used nodeclass because the attribute I have to set on the classifier is IS_NODECLASS. But I'm happy to change it to whatever you prefer. What about "classification"? And should we keep all elements?

discordianfish commented 7 years ago

@byxorna / @alex-laties Can we get this merged?

byxorna commented 7 years ago

@discordianfish could you change it to "CLASSIFICATION" before merging?

discordianfish commented 7 years ago

@byxorna Done!

byxorna commented 7 years ago

:shipit: (cc @Primer42)

michaeljs1990 commented 7 years ago

anyway to get this bumped? This would help me stop severe and everlasting damage from befalling our puppet repo.

I did run the tests locally as well to make sure this wasn't breaking anything and everything passed.

byxorna commented 7 years ago

cc @roymarantz @michaeljs1990

This imho should get unit tests illustrating basic behavior validation, as well as a bit in the docs describing this thing. @discordianfish are you willing to make those changes? (i.e. at least illustrate no classification serializes to null).

discordianfish commented 7 years ago

I'm not using collins anymore, so won't have time for this unfortunately.

michaeljs1990 commented 7 years ago

@discordianfish thanks for the reply i'll take this over then just didn't want to steal it from you.