tumblr / collins

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

Dynamic Enum Fix #547

Closed michaeljs1990 closed 7 years ago

michaeljs1990 commented 7 years ago

Adds support so that when a value that is of type Dynamic Enum is updated after initial intake the values are cleared from the initial run and the new ones are added.

See #546

michaeljs1990 commented 7 years ago

This needs tests still but I wanted to make sure others were ok with this change before doing so.

byxorna commented 7 years ago

@michaeljs1990 what kind of test cases can be added to prove behavior?

michaeljs1990 commented 7 years ago

I was going to first induct a machine and then update the asset with the same LSHW except for passing in a different value for BaseDescription, BaseProduct, BaseVendor, and BaseSerial in the xml. Currently it would show the original values from the initial induction process as well as having two entries in the database for each of the above attached to the asset.

byxorna commented 7 years ago

@michaeljs1990 if we can do the induction in a test, that would be ideal. That way we know nothing breaks now, and in the future with more changes that will be likely made to this codebase.

michaeljs1990 commented 7 years ago

I think the added tests should cover all cases. Wasn't sure where to put this test though it makes use of the controller helpers a bunch but it's actually testing a sub section of AssetMeta.

byxorna commented 7 years ago

Ive kicked the tests again, because i think 2017-05-21 07:02:08,078 - [ERROR] - SolrKey - p.a.LoggerLike$class:error:131 - Cannot create sortable multivalued key for %s, forcing non-sortable is caused by a race.

byxorna commented 7 years ago

@defect @roymarantz can you take a look? This is good for landing

michaeljs1990 commented 7 years ago

Been running this in prod for a few weeks now without issue side by side with the latest GPU changes.

byxorna commented 7 years ago

If we dont hear anyone reject this in a few days, I'll land this. This is an awesome fix!

michaeljs1990 commented 7 years ago

🔥 🌶 🐝