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

virtual column for parent blue folder path with excluded non-display folders #17976

Conversation

AparnaKarve
Copy link
Contributor

The new virtual column has been added to make the v_folder_path_no_non_display_folders attribute accessible via the api/vms API.

In v2v UI, we currently display the path info via API for VMs which internally calls this method - https://github.com/ManageIQ/manageiq/blob/master/app/models/transformation_mapping.rb#L83

We are currently in the process of adding Edit support for Plans, where we will need this attribute accessible to us via api/vms API

@AparnaKarve
Copy link
Contributor Author

@bzwei Can you please review?

@bzwei
Copy link
Contributor

bzwei commented Sep 11, 2018

I would recommend making change in https://github.com/ManageIQ/manageiq/blob/master/app/models/transformation_mapping.rb#L87 to utilize this virtual column in this PR.

@AparnaKarve AparnaKarve force-pushed the virtual_col_blue_path_exclude_non_display_folders branch from f9521f8 to 552e782 Compare September 11, 2018 19:28
@AparnaKarve
Copy link
Contributor Author

@bzwei Per your recommendation, I have utilized the virtual column in the describe_vm method.

@bzwei
Copy link
Contributor

bzwei commented Sep 12, 2018

LGTM, wait for CI passed.

@AparnaKarve AparnaKarve reopened this Sep 12, 2018
@mturley
Copy link
Contributor

mturley commented Sep 12, 2018

@bzwei, @AparnaKarve are we ready to merge this? It's the last piece I need to finish ManageIQ/manageiq-v2v#620.

Thanks so much for getting this in so quickly!

@AparnaKarve
Copy link
Contributor Author

@gmcculloug Can you review and merge?

@@ -803,6 +804,11 @@ def parent_datacenter
end
alias_method :owning_datacenter, :parent_datacenter

def parent_blue_folder_path_with_excluded_non_display_folders
Copy link
Member

Choose a reason for hiding this comment

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

I think something like parent_blue_folder_display_path would be a better name

@@ -151,6 +151,7 @@ class VmOrTemplate < ApplicationRecord
virtual_column :v_owning_blue_folder, :type => :string, :uses => :all_relationships
virtual_column :v_owning_blue_folder_path, :type => :string, :uses => :all_relationships
virtual_column :v_datastore_path, :type => :string, :uses => :storage
virtual_column :v_folder_path_no_non_display_folders, :type => :string, :uses => :all_relationships
Copy link
Member

Choose a reason for hiding this comment

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

All of the other virtual_columns for the parent_blue_folders are called parent_blue_folder_something ref so to keep it consistent how about v_parent_blue_folder_display_path ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure... as long as the name fits in that space that the other virtual columns are using
-- and it does!

I have renamed the method and the virtual column per your suggestion.
Thanks.

@AparnaKarve AparnaKarve force-pushed the virtual_col_blue_path_exclude_non_display_folders branch from 552e782 to 26289cb Compare September 12, 2018 17:24
@AparnaKarve AparnaKarve force-pushed the virtual_col_blue_path_exclude_non_display_folders branch from 26289cb to d3709e0 Compare September 12, 2018 17:25
@miq-bot
Copy link
Member

miq-bot commented Sep 12, 2018

Checked commit AparnaKarve@d3709e0 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

app/models/vm_or_template.rb

@mturley
Copy link
Contributor

mturley commented Sep 12, 2018

@agrare Thanks for the review!
@gmcculloug Any chance we can merge this today?
Thanks again to all, sorry to bother you guys.

@gmcculloug gmcculloug merged commit 5d75759 into ManageIQ:master Sep 12, 2018
@gmcculloug gmcculloug added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 12, 2018
@AparnaKarve AparnaKarve deleted the virtual_col_blue_path_exclude_non_display_folders branch September 12, 2018 19:42
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.

6 participants