Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding custom attributes to provider (UI + API) #10897

Merged
merged 2 commits into from
Oct 6, 2016

Conversation

alongoldboim
Copy link
Contributor

@alongoldboim alongoldboim commented Aug 31, 2016

New view
selection_101

Added a new api endpoint:
url: http://localhost:3000/api/providers/3/custom_attributes
method: Post

Request.payload:

{
    "action": "add/edit/delete",
    "resources": [{
        "name": "exp_date",
        "value": "1.2.2016",
        "field_type": "Date"
    }, {
        "name": "expected_num_of_nodes",
        "value": 2
    }, {
        "name": "sales_force_acount",
        "value": "ADF231VRWQ1"
    }]
}

@alongoldboim alongoldboim force-pushed the provider_meta_data branch 6 times, most recently from 67ca6a4 to 67397b7 Compare August 31, 2016 11:30
@alongoldboim
Copy link
Contributor Author

depends on: #10896

CONNECTION_ATTRS = %w(connection_configurations).freeze
ENDPOINT_ATTRS = %w(hostname ipaddress port security_protocol).freeze
RESTRICTED_ATTRS = [TYPE_ATTR, CREDENTIALS_ATTR, ZONE_ATTR, "zone_id"]
RESTRICTED_ATTRS = [TYPE_ATTR, CREDENTIALS_ATTR, ZONE_ATTR, "zone_id"].freeze
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't mix unrelated rubocops in the same PR and the feature itself (i.e. these freezes).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

@Fryguy
Copy link
Member

Fryguy commented Aug 31, 2016

So, I think we already have methods for adding and removing custom attributes (perhaps not via the REST API). I'm not sure why we need to re-add that under a new name of "meta data" cc @gmcculloug I think you've done this before, right?

def meta_data_resource_providers(_type, _id = nil, data)
provider = ExtManagementSystem.find_by_name(data.delete("name"))
data.each do |key, value|
attribute = provider.custom_attributes.find_by_name(key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an N+1. I think you can query all of the requisite attributes up front (or just query them all)

@Fryguy
Copy link
Member

Fryguy commented Aug 31, 2016

an action name of "meta_data" by itself is very strange. Since it does a full replacement (but no deletes?) I would expect an action word in the front like "update_metadata"

Also,

  • metadata is a single word, so shouldn't be broken up
  • even so (as mentioned above) I think it should say custom_attributes to be consistent with usages in the UI.

@Fryguy
Copy link
Member

Fryguy commented Aug 31, 2016

Sorry, one more thing...we also have the concept of different types of custom_attributes as handled by the "source" column. So ManageIQ can have it's own custom_attributes which I think uses the MIQ source, and the EMS can have theirs which I think is the EMS source, with possibilities of other sources. I think you might want to limit which source you want to show in the UI. Again, I believe this is handled on the VMs screen, but possibly others...I'd have to check to be sure

@gmcculloug
Copy link
Member

You can see this on the VM screen by editing any VM in the UI and setting the Custom Identifier field to value.

image

It will show up in the Custom Attributes section of the VM Summary screen as custom_1
image

@Fryguy Fryguy mentioned this pull request Aug 31, 2016
@alongoldboim alongoldboim force-pushed the provider_meta_data branch 4 times, most recently from 4b4ef23 to e9e2810 Compare September 1, 2016 10:27
@imtayadeway
Copy link
Contributor

@abellotti If I understood correctly I think this action is supported in current implementation?

Are there tests to support that?

@alongoldboim
Copy link
Contributor Author

alongoldboim commented Oct 5, 2016

@abellotti fixed minor comments, @imtayadeway Yes the current tests are supporting add/edit/delete.
@gtanzillo @Loicavenel Think this one can get merged if there are no more comments.

@alongoldboim
Copy link
Contributor Author

@miq-bot add_label euwe/yes

@abellotti
Copy link
Member

Hi @alongoldboim thanks for the updates. could you take of the rubocop warnings ? Thanks.

@gtanzillo
Copy link
Member

👍 LGTM. Will merge once the rubocops are addressed.

@miq-bot
Copy link
Member

miq-bot commented Oct 6, 2016

Checked commits alongoldboim/manageiq@66b37e3~...e406e78 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
5 files checked, 0 offenses detected
Everything looks good. ⭐

@alongoldboim alongoldboim reopened this Oct 6, 2016
@alongoldboim
Copy link
Contributor Author

@gtanzillo Rubocop is happy now.

@gtanzillo gtanzillo added this to the Sprint 48 Ending Oct 24, 2016 milestone Oct 6, 2016
@gtanzillo gtanzillo merged commit 46277ee into ManageIQ:master Oct 6, 2016
chessbyte pushed a commit that referenced this pull request Oct 13, 2016
Adding custom attributes to provider (UI + API)
(cherry picked from commit 46277ee)
@chessbyte
Copy link
Member

Euwe Backport details:

$ git log
commit a16aedc889a2bec9e37a2530b530f46483b31d33
Author: Gregg Tanzillo <[email protected]>
Date:   Thu Oct 6 11:08:03 2016 -0400

    Merge pull request #10897 from alongoldboim/provider_meta_data

    Adding custom attributes to provider (UI + API)
    (cherry picked from commit 46277eefa4ea925a2607628c7815fbc5a8f214c0)

jrafanie added a commit to Fryguy/manageiq that referenced this pull request May 1, 2017
For now, make the test expectations different for ruby 2.4.0+ vs. prior rubies.

CustomAttribute#value_type returns a symbol for a ruby core class, a
hardcoded value instead of the class's behavior.
Do we use this?  Why are we returning a value instead of caring about
behavior of the old Fixnum and Integer?  If we don't use this, we
should remove the whole method and it's tests added in:
ManageIQ@4687394

The PR that added this was:
ManageIQ#11000

Adding CustomAttribute to provider UI/API was added in:
ManageIQ#10897
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants