Add support for validate_vms action on transformation_mappings

[abellotti]

Extends #352

Dependent on ManageIQ/manageiq#17177

Supports import from csv or without any data at all

POST /api/transformation_mappings/:id
{
   "action": "validate_vms"
}

POST /api/transformation_mappings/:id
{
   "action": "validate_vms",
   "import": [
      { "name": "vm_name", "uid": "vm_uid" } 
    ]
}

cc: @gtanzillo @bzwei

[abellotti]

Marking as WIP as we need ManageIQ/manageiq#17177 merged first

resolves issue #352 (comment)

so results now would return vms hrefs, so:

{
    "valid_vms": [
        {
            "href": "http://localhost:3000/api/vms/2222",
            "name": "amaya-at",
            "cluster": "Default",
            "path": "RHV/Default",
            "allocated_size": "32212254720.0",
            "id": "2222",
            "reason": "OK"
        }
    ]
}

[abellotti]

@gmcculloug had to update the specs to have the proper wiring for the valid vm case.

ping @imtayadeway can we borrow your 👀 for review ? Thanks.

[imtayadeway]

This mostly looks good although I had some concerns about adding complexity for one edge case (that lives in a concrete sub-type 🎉 ), as well as blindly returning bad requests for any standard error. More details below. Thanks @abellotti ! 🎹

[imtayadeway]

It seems unfortunate that we have to add more params to the already quite-complex normalize_<thing> methods for this one edge case. For example, the proliferation of items and types in https://github.com/ManageIQ/manageiq-api/pull/358/files#diff-d4f69c54bfacef864c9985012cdb05dfR131 requires a lot of context to grok! This also seems to be obscuring intent - we are saying "normalize this thing as a vm" but this resource isn't a vm, it's a bundle of data that contains some vm attributes, and some others.

I'm assuming then that the reason this is done is to append the href to the individual valid/invalid/conflict groups? WDYT about doing this directly here, removing the need to add more complexity to the above methods:

    def validate_vms_resource(type, id, data = {})
      transformation_mapping = resource_search(id, type, collection_class(type))
      (transformation_mapping.validate_vms(data["import"]) || {}).tap do |res|
        %w(valid_vms invalid_vms conflict_vms).each do |key|
          next unless res.key?(key)
          res[key].each do |v|
            v["href"] = normalize_href(:vms, v["id"])
          end
        end
      end
    rescue StandardError => err
      raise BadRequestError, "Could not validate vms - #{err}"
    end

[abellotti]

This is fine too for now, but there is still an issue with the normalize_ methods where type is not properly propagated and type is deduced from the request when we explicitly are telling normalize_hash the type.

[imtayadeway]

Do we really want to rescue a StandardError here and tell the user they made a bad request? It seems like we should anticipate the things that could go wrong and respond with a bad request if the user made an error, otherwise the error is on us.

It might be nice to see some tests around this path illustrating what a user would have to do to get this response.

[abellotti]

Not sure we can anticipate what can wrong, this is outside API's knowledge and the validate_vms does not raise any malformed input error for now, so really nothing to catch and make a BadRequestError of. we should probably just drop this rescue and resort to the 500.

[miq-bot]

Checked commits abellotti/manageiq-api@cceac71~...e1d4eaf with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

[abellotti]

updated & re-pushed @imtayadeway

/cc @AparnaKarve

[gtanzillo]

@imtayadeway Can we get this one merged? If there are additional changes that we need, we can do that in a followup PR.

[imtayadeway]

I'm not sure why this postfix conditional is needed - isn't it part of the contract of the return value of validate_vms that it will have an id, and that it won't have an href? If there are cases where this is needed, this should have a test. Perhaps you could handle this in a follow up PR? Thanks @abellotti !

[imtayadeway]

LGTM - I have just one follow-up item here but I think this is good enough to merge. Thanks @abellotti !

[gmcculloug]

@miq-bot add_label transformation

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit eeeb1992d5ff128ef170517d20d4ee7a50f0ff07
Author: Tim Wade <[email protected]>
Date:   Mon Apr 30 07:40:47 2018 -0700

    Merge pull request #358 from abellotti/transformation_mapping_validate_vms
    
    Add support for validate_vms action on transformation_mappings
    (cherry picked from commit 94370162a7b6e7e6d896fac21f7cc2b09747c127)