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 detection of custom attributes field type in miq expression #11000

Conversation

alongoldboim
Copy link
Contributor

Added field_type to custom-attributes in: #10897, in this pr i have modified the expression builder in polices to parse the custom-attribute by its field type.

@alongoldboim alongoldboim changed the title adding detection of custom attributes field type in miq expression [WIP] Adding detection of custom attributes field type in miq expression Sep 4, 2016
@alongoldboim alongoldboim force-pushed the enable_custom_attributes_type_def_in_miq_expression branch 9 times, most recently from db07fd0 to 784892c Compare September 6, 2016 07:33
@alongoldboim alongoldboim changed the title [WIP] Adding detection of custom attributes field type in miq expression Adding detection of custom attributes field type in miq expression Sep 6, 2016
@@ -1334,10 +1334,19 @@ def self.get_col_type(field)

model = determine_model(model, parts)
return nil if model.nil?
return custom_attribute_field_type(col) if col.include?("virtual_custom_attribute")
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use ...::CUSTOM_ATTRIBUTES_PREFIX in custom attributes mixin

@lpichler
Copy link
Contributor

lpichler commented Sep 6, 2016

@alongoldboim is this PR still wip ? it would be nice to add some test case, let is see also what is it exactly doing.

@alongoldboim alongoldboim force-pushed the enable_custom_attributes_type_def_in_miq_expression branch 2 times, most recently from b243246 to 656b549 Compare September 6, 2016 10:36
@alongoldboim alongoldboim changed the title Adding detection of custom attributes field type in miq expression [WIP] Adding detection of custom attributes field type in miq expression Sep 6, 2016
@alongoldboim
Copy link
Contributor Author

@lpichler working on the tests part, will remove WIP once done.

@alongoldboim alongoldboim force-pushed the enable_custom_attributes_type_def_in_miq_expression branch 2 times, most recently from 4c584a8 to 015e939 Compare September 6, 2016 12:11
@alongoldboim alongoldboim changed the title [WIP] Adding detection of custom attributes field type in miq expression Adding detection of custom attributes field type in miq expression Sep 6, 2016
@alongoldboim
Copy link
Contributor Author

@lpichler tests were added, please review :)

@alongoldboim alongoldboim force-pushed the enable_custom_attributes_type_def_in_miq_expression branch from 015e939 to 8187f08 Compare September 6, 2016 12:20
@alongoldboim
Copy link
Contributor Author

Depends on: #10896

@Fryguy
Copy link
Member

Fryguy commented Sep 6, 2016

@imtayadeway Note that this relies on the database migration changes in #10896, so you may have some opinion there as well.

field_type = CustomAttribute.find_by_name(col.gsub(CustomAttributeMixin::CUSTOM_ATTRIBUTES_PREFIX, "")).field_type
field_type.to_sym
rescue
_log.info("Couldn't detect custom-attribute: #{col} feild type")
Copy link
Contributor

Choose a reason for hiding this comment

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

/s/feild/field/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixing :)

@imtayadeway
Copy link
Contributor

@alongoldboim just a couple more questions above, otherwise LGTM 😄

@alongoldboim alongoldboim force-pushed the enable_custom_attributes_type_def_in_miq_expression branch from 6569330 to ac6faa0 Compare September 26, 2016 13:17
@alongoldboim
Copy link
Contributor Author

@imtayadeway I think i got them all :)

@alongoldboim alongoldboim force-pushed the enable_custom_attributes_type_def_in_miq_expression branch 2 times, most recently from 4fb539a to d9a6ac2 Compare September 27, 2016 08:08
@imtayadeway
Copy link
Contributor

@alongoldboim 👍 LGTM. Can you squash your refactor commit or otherwise give it a more descriptive commit message? Thanks!

@alongoldboim alongoldboim force-pushed the enable_custom_attributes_type_def_in_miq_expression branch from d9a6ac2 to 4687394 Compare September 27, 2016 11:11
@alongoldboim
Copy link
Contributor Author

@imtayadeway Done.

@miq-bot
Copy link
Member

miq-bot commented Sep 27, 2016

Checked commit alongoldboim@4687394 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
5 files checked, 3 offenses detected

spec/models/custom_attribute_spec.rb

@gtanzillo
Copy link
Member

👍

@gtanzillo gtanzillo added this to the Sprint 47 Ending Oct 3, 2016 milestone Sep 28, 2016
@gtanzillo gtanzillo merged commit 58521b1 into ManageIQ:master Sep 28, 2016
@chessbyte
Copy link
Member

@gtanzillo please label euwe/yes or euwe/no

@imtayadeway
Copy link
Contributor

@miq-bot add-label euwe/yes

chessbyte pushed a commit that referenced this pull request Oct 13, 2016
…type_def_in_miq_expression

Adding detection of custom attributes field type in miq expression
(cherry picked from commit 58521b1)
@chessbyte
Copy link
Member

Euwe Backport details:

$ git log
commit e9562ca7eacd56d2d47c91240852cfb240c39842
Author: Gregg Tanzillo <[email protected]>
Date:   Wed Sep 28 11:00:43 2016 -0400

    Merge pull request #11000 from alongoldboim/enable_custom_attributes_type_def_in_miq_expression

    Adding detection of custom attributes field type in miq expression
    (cherry picked from commit 58521b147b364254b1de8b47d45739ef19bea915)

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.

7 participants