tumblr / collins

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

gui delete fix #383

Closed gtorre closed 8 years ago

gtorre commented 8 years ago

@maddalab @Primer42 @roymarantz @defect rfr

Right now the delete action on the web gui does not work, it simply places the asset in decommissioned. This has been tested and works.

yl3w commented 8 years ago

Can you provide a unit test please?

roymarantz commented 8 years ago

is there still a way to decommission a node?

gtorre commented 8 years ago

@roymarantz sure, you have to change the status to Decommissioned.

william-richard commented 8 years ago

:-1: I'm not sure this is a good idea. Nuking should not be the default. Nuking an asset is an exceptional action, and it should not be an easy thing to do. It should require thought, and so it should either be a different button (that sends up a big prompt) or only available over the API.

byxorna commented 8 years ago

I agree. I was under the impression that this functionality was only being added to the API, and the default delete action would be the same as legacy functionality (decommission asset, not nuke!). The GUI should only decommission imho.

gtorre commented 8 years ago

@byxorna @Primer42

A few months ago I wrote a script to delete unused VM assets. At the time there was no API endpoint that would allow me to do this, so I introduced the 'nuke' endpoint. Before I even made any changes, administrators (the list maintained in puppet) could delete (COMPLETE delete, aka nuke) assets from Collins using the web GUI.

Yesterday I tried to help Vincent delete an asset from Collins, and noticed that it simply moved it to decommissioned. This is not the same behavior from two months ago, because when I introduced the 'nuke' feature I essentially broke the GUI portion because we now must pass a nuke variable for assets to actually delete.

This change brings the GUI back to the original state where we could use the GUI to delete assets. I'm not introducing anything new, and people can't just delete assets on a whim.. they must a. be administrators, and b. click on a scary red button after entering a reason for doing so.

I just wanted to make sure that everyone had the correct facts before we decide to do away with a feature that was already in place two months ago and everyone was ok with at the time :)

byxorna commented 8 years ago

@gtorre before your changes, the GUI would decomm an asset, and the API would decomm the asset. The intent of your inital change is to make the API either decomm or nuke, depending on the nuke parameter. The default was decomm. This change makes the GUI delete, which was not the intent of your first change, thus the issues being raised.

tl;dr: why isnt this fixing decommissioning and not changing the default behavior of the GUI?

byxorna commented 8 years ago

@gtorre any progress on this? Would be nice if there was a popup when you click delete (only for admins!) that asks if you wanna nuke or just decomm.

gtorre commented 8 years ago

@byxorna this already exists:

screenshot 2016-02-17 22 00 21

I'm not sure what else to do with this... I kind of forgot about it because you and Will were strongly against re-adding the feature in the web UI although I firmly believe it was always available to admins. To prove this, I removed blake from the admins section of conf/permissions.yaml and added a few test assets which I then tried to delete:

screenshot 2016-02-17 22 13 08

As you can see, there isn't an option to delete the asset. Which means, unless the user is given admin rights they cannot delete the asset. This P/R was meant to fix what I had broken.

byxorna commented 8 years ago

@gtorre What I meant is that, for admins, there should be an option in that first screenshot to either nuke, or just decommission (soft delete i suppose?). As it stands now, there is no way to do the old-style soft delete via web UI.

gtorre commented 8 years ago

@byxorna what about Maintenance Start -> Decommissioned

That essentially does the same thing.

byxorna commented 8 years ago

@gtorre fair point. That sounds good to me

lqmccoy commented 8 years ago

@gtorre It's not really a maintenance. Therefore I don't believe it belongs there. Decom should have its only place in the Actions drop down.

yl3w commented 8 years ago

I with @lqmccoy on this one @gtorre. @byxorna was right before you should fix it

lqmccoy commented 8 years ago

@gtorre any updates on this?