Bug 1058549
Summary: | Key value import gives 500 error for undefined systems | ||
---|---|---|---|
Product: | [Retired] Beaker | Reporter: | xjia <xjia> |
Component: | web UI | Assignee: | Amit Saha <asaha> |
Status: | CLOSED CURRENTRELEASE | QA Contact: | tools-bugs <tools-bugs> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | develop | CC: | aigao, asaha, dcallagh, ebaak, jingwang, llim, qwan, rmancy, xtian |
Target Milestone: | 0.15.4 | ||
Target Release: | --- | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2014-02-18 01:38:44 UTC | Type: | Bug |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
xjia
2014-01-28 02:45:26 UTC
I am wondering about the correct fix for this. I see two options: - Fail to import with an error that the system doesn't exit - Create the system like we do for others and populate it with the key value. For all of the CSV types other than "Systems" and "Systems (for modification)", failing with a 422 Unprocessable Entity for unknown FQDN entries would be ideal (and the same goes for unknown non-empty system IDs in the latter case). However, due to the current limitations of the CSV import page, a 200-with-an-error-message response may be the best we can do for now (that's what is currently done to report CSV import failures and it isn't worth investing in changing that just to resolve this bug). Also, ensure that the handling of missing FQDNs is tested for all of the other CSV import types. Implicitly creating missing systems is acceptable (it isn't worth the hassle of deprecating that behaviour in cases where it already happens), but any other cases where it currently triggers 500 errors should also be updated to report a proper error message. It turns out that the CSV import for non-existent systems creates the system if it doesn't exist for all the other import types: labinfo, power, install, exclude, groups barring keyvalue. It seems to me that there may have been an oversight in the key value import code. So, I have taken the route of fixing this oversight rather than barring it, so that it is consistent with the others. Preliminary patch: http://gerrit.beaker-project.org/#/c/2759/1 OK, after reviewing Amit's patch, it's (much) clearer that the original intent of the CSV import process was to always implicitly create the system if it wasn't already there, regardless of the kind of CSV record involved. While I still have reservations about that design choice (although it does have the benefit of making a full import of a particular system independent of the order of the rows), I agree with Amit that the correct solution at this point in time is to simply bring the Key/Value import into line with the handling of other system rows, rather than moving to an approach where there is a "primary" system row type that must be provided first in order to create a new system. Beaker 0.15.4 has been released. |