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

Return NODECLASS in API #482

Closed

Conversation

discordianfish
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

byxorna commented Oct 30, 2016

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@byxorna
Copy link
Contributor

byxorna commented Nov 17, 2016

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

@discordianfish
Copy link
Contributor Author

@byxorna Done!

@byxorna
Copy link
Contributor

byxorna commented Nov 18, 2016

:shipit: (cc @Primer42)

@michaeljs1990
Copy link
Contributor

michaeljs1990 commented Apr 8, 2017

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
Copy link
Contributor

byxorna commented Apr 13, 2017

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
Copy link
Contributor Author

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

@michaeljs1990
Copy link
Contributor

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

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