tumblr / collins

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

Skip UID verification when ipmi config is missing #492

Closed discordianfish closed 4 years ago

discordianfish commented 7 years ago

This will make collins' intake process skip UID based identification when IPMI data is missing.

Let me know if you need this configurable.

roymarantz commented 7 years ago

Shouldn't there be a config "feature" test enabling this change in behavior? Ideally there would be unit tests too.

discordianfish commented 7 years ago

@roymarantz 'config "feature" test'? You mean adding some flag to app/collins/intake/IntakeConfig.scala like strict_validation:

Tests would be ideally but I'm not very familiar with scala and there are no existing tests for the intake stuff I could extend easily. If you can point me in the right direction, I can look into this though.

roymarantz commented 7 years ago

By feature I mean something like https://tumblr.github.io/collins/configuration.html#features and a simple example of its use can be found in app/collins/util/StateMachines.scala just look for Feature. I can't find an instance of a similar kind of test 😞 Is there an advantage to adding such a feature test in app/collins/intake/IntakeConfig.scala instead of where your code change is?

discordianfish commented 7 years ago

Ah, i see. Yes such feature works for me. Would call it sloppyIntake?

roymarantz commented 7 years ago

The name doesn't matter to much, but I'd favor something more descriptive like optionalIpmiIntake

discordianfish commented 7 years ago

@roymarantz Like this?

I also want to add another feature to allow skipping chassis verification as well, so might want to wait before merging this.

roymarantz commented 7 years ago

this looks fine with me 👍 I'd suggest adding the other feature in another PR unless you believe it is tightly linked to this one.

discordianfish commented 7 years ago

Fine with me. Then this is ready to get merged!

discordianfish commented 7 years ago

@Primer42 Ping?