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.

Sign up for GitHub

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

Jump to bottom

storage service endpoint #8617

Merged
merged 1 commit into from
Feb 10, 2023
Merged

storage service endpoint #8617

merged 1 commit into from
Feb 10, 2023

Conversation

OrGur1987
Copy link
Contributor

@OrGur1987 OrGur1987 commented Jan 29, 2023 •
edited

see issue #8616

also, this PR includes changes based on enhancements from PR #8631, and adds service_storage related enhancements.
this will probably require a rebase once #8631 is merged

@OrGur1987 OrGur1987 requested a review from a team as a code owner January 29, 2023 09:01
@OrGur1987 OrGur1987 mentioned this pull request Jan 29, 2023
Closed
5 tasks
@miq-bot miq-bot added the wip label Jan 29, 2023
@OrGur1987 OrGur1987 force-pushed the storage_services_endpoint branch 3 times, most recently from 3f63ab5 to 66af393 Compare January 29, 2023 16:23
@OrGur1987 OrGur1987 changed the title [WIP] storage service endpoint storage service endpoint Jan 31, 2023
@miq-bot miq-bot removed the wip label Jan 31, 2023
@OrGur1987
Copy link
Contributor Author

Checked commit Autosde@66af393 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint 20 files checked, 11 offenses detected

app/helpers/application_helper/button/storage_service_new.rb

app/helpers/ems_storage_helper/textual_summary.rb

app/helpers/storage_service_helper/textual_summary.rb

app/views/storage_service/new.html.haml

  • ⚠️ - Line 3 - Layout/TrailingEmptyLines: Final newline missing.

app/views/storage_service/show.html.haml

  • ⚠️ - Line 1 - Layout/TrailingEmptyLines: Final newline missing.

app/views/storage_service/show_list.html.haml

  • ⚠️ - Line 1 - Layout/TrailingEmptyLines: Final newline missing.
  • ⚠️ - Line 1 - id attribute must be in lisp-case

these errors refer to formats I copied from existing files

https://github.com/ManageIQ/manageiq-ui-classic/actions/runs/4037650069/jobs/6941095157
this may need a cross-repo test with the main manageiq repo for MiqProductFeature

@agrare agrare self-assigned this Feb 1, 2023
@OrGur1987 OrGur1987 force-pushed the storage_services_endpoint branch 3 times, most recently from 1623955 to a1ad580 Compare February 5, 2023 09:41
@jeffibm
Copy link
Member

jeffibm commented Feb 6, 2023

Hey @OrGur1987 , could you also have a look into the failing specs..

@jeffibm jeffibm closed this Feb 6, 2023
@jeffibm jeffibm reopened this Feb 6, 2023
@OrGur1987
Copy link
Contributor Author

Hey @OrGur1987 , could you also have a look into the failing specs..

hi, could u please re-run the cross repo checks?
thanks

@OrGur1987 OrGur1987 mentioned this pull request Feb 6, 2023
Merged
@OrGur1987 OrGur1987 force-pushed the storage_services_endpoint branch 2 times, most recently from 8d399cf to 8d167ad Compare February 6, 2023 09:51
@OrGur1987
Copy link
Contributor Author

hi @jeffibm,
can you re-run the cross repo test?
thanks

@OrGur1987
Copy link
Contributor Author

Hey @OrGur1987 , could you also have a look into the failing specs..

hi, could u please re-run the cross repo checks? thanks

@jeffibm
did you run it with the branch from this issue?
ManageIQ/manageiq#22331

@jeffibm
Copy link
Member

jeffibm commented Feb 7, 2023

Hey @OrGur1987 , could you also have a look into the failing specs..

hi, could u please re-run the cross repo checks? thanks

@jeffibm did you run it with the branch from this issue? ManageIQ/manageiq#22331

ohh, we have a branch in core to be merged right? I just re-run all the jobs before.

@OrGur1987
Copy link
Contributor Author

Hey @OrGur1987 , could you also have a look into the failing specs..

hi, could u please re-run the cross repo checks? thanks

@jeffibm did you run it with the branch from this issue? ManageIQ/manageiq#22331

ohh, we have a branch in core to be merged right? I just re-run all the jobs before.

yes, I've added storage_services to miq_product_features and miq_user_roles in the branch in the main repo

@jeffibm
Copy link
Member

jeffibm commented Feb 7, 2023

Hey @OrGur1987 , could you also have a look into the failing specs..

hi, could u please re-run the cross repo checks? thanks

@jeffibm did you run it with the branch from this issue? ManageIQ/manageiq#22331

ohh, we have a branch in core to be merged right? I just re-run all the jobs before.

yes, I've added storage_services to miq_product_features and miq_user_roles in the branch in the main repo

Hey @OrGur1987 , could you also have a look into the failing specs..

hi, could u please re-run the cross repo checks? thanks

@jeffibm did you run it with the branch from this issue? ManageIQ/manageiq#22331

ohh, we have a branch in core to be merged right? I just re-run all the jobs before.

yes, I've added storage_services to miq_product_features and miq_user_roles in the branch in the main repo

ok, I am not sure how to run the jobs without the core branch being merged. @agrare , could you please help..

@agrare
Copy link
Member

agrare commented Feb 7, 2023

ok, I am not sure how to run the jobs without the core branch being merged. @agrare , could you please help..

@jeffibm that is what cross-repo-tests does. See https://github.com/ManageIQ/miq_bot for examples

@agrare
Copy link
Member

agrare commented Feb 7, 2023

@miq-bot cross-repo-tests ManageIQ/manageiq#22331

@miq-bot miq-bot mentioned this pull request Feb 7, 2023
Open
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Feb 7, 2023
@agrare @miq-bot
Running tests for agrare
ea049a5 
From Pull Request:  ManageIQ/manageiq-ui-classic#8617
agrare
agrare reviewed Feb 8, 2023
View reviewed changes
app/helpers/application_helper.rb
@@ -251,7 +251,7 @@ def view_to_url(view, parent = nil)
if controller == "ems_storage" && action == "show"
return ems_storages_path
end

Copy link
Member

Choose a reason for hiding this comment

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

Oooo good find

agrare
agrare reviewed Feb 8, 2023
View reviewed changes
app/helpers/application_helper/button/storage_service_new.rb Outdated
# Else disable button if no active providers support create action
def disabled?
if @view_context.params["id"]
current_ems = ExtManagementSystem.where(:id => @view_context.params["id"]).first
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
current_ems = ExtManagementSystem.where(:id => @view_context.params["id"]).first
current_ems = ExtManagementSystem.find(@view_context.params["id"])

or If you don't want to raise an exception if we can't find by ID

Suggested change
current_ems = ExtManagementSystem.where(:id => @view_context.params["id"]).first
current_ems = ExtManagementSystem.find_by(:id => @view_context.params["id"])

but based on the next line I think you want to raise if it can't find one since you're not checking for nil

agrare
agrare reviewed Feb 8, 2023
View reviewed changes
app/helpers/application_helper/button/storage_service_new.rb Outdated
Comment on lines 16 to 18
ExtManagementSystem.none? do |ems|
ems.class_by_ems("StorageService")&.supports?(:create)
end
Copy link
Member

Choose a reason for hiding this comment

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

Double-check me on this but I think StorageService.providers_supporting(:create).none? would work better (it filters EMS by type-supporting first so more performant)

agrare
agrare reviewed Feb 8, 2023
View reviewed changes
app/helpers/cloud_volume_helper/textual_summary.rb Outdated
@@ -61,6 +61,12 @@ def textual_storage_resource
nil
end

def textual_storage_service
@record.ext_management_system.storage_services.find(@record.storage_service_id)
Copy link
Member

Choose a reason for hiding this comment

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

If you have an ID can't you just do StorageService.find(@record.storage_service_id) or better yet do you have @record.storage_service ?

agrare
agrare reviewed Feb 8, 2023
View reviewed changes
app/helpers/storage_resource_helper/textual_summary.rb Outdated
"#{capability['name']}: #{capability['value']}"
def textual_group_cloud_volumes
volumes = @record.cloud_volumes.map do |v|
storage_service = @record.ext_management_system.storage_services.find(v.storage_service_id)
Copy link
Member

Choose a reason for hiding this comment

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

Similar question, can you just do StorageService.find(v.storage_service_id) or use v.storage_service ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw, we notices that the textual_link don't work in the resulting tables.
it looks like a link (blue) but clicking it gets no response.
I suspect the map function somehow disables it..

opened a bug-ticket in our next sprint to try and fix this throughout our provider.

agrare
agrare reviewed Feb 8, 2023
View reviewed changes
app/helpers/storage_service_helper/textual_summary.rb Outdated

def textual_group_storage_resources
resources = @record.storage_resources.map do |r|
free_space = number_to_percentage(r.logical_free.to_f * 100 / r.logical_total.to_f, :precision => 1)
Copy link
Member

@agrare agrare Feb 8, 2023 •
edited

Choose a reason for hiding this comment

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

Shouldn't we check that r.logical_total.to_f isn't 0? Technically N/0.0 float is Infinity but not sure how number_to_percentage handles that ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it now looks like this if logical_total is 0:
image

agrare
agrare reviewed Feb 8, 2023
View reviewed changes
app/helpers/storage_service_helper/textual_summary.rb Outdated

def textual_group_cloud_volumes
volumes = @record.cloud_volumes.map do |v|
storage_resource = @record.ext_management_system.storage_resources.find(v.storage_resource_id)
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as https://github.com/ManageIQ/manageiq-ui-classic/pull/8617/files#r1100693975

@miq-bot
Copy link
Member

miq-bot commented Feb 9, 2023

Checked commit Autosde@b58043a with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
23 files checked, 18 offenses detected

app/helpers/application_helper/button/storage_service_new.rb

app/helpers/cloud_volume_helper/textual_summary.rb

app/helpers/ems_storage_helper/textual_summary.rb

app/helpers/storage_resource_helper/textual_summary.rb

app/helpers/storage_service_helper/textual_summary.rb

app/views/storage_service/show.html.haml

  • ⚠️ - Line 1 - Layout/TrailingEmptyLines: Final newline missing.

app/views/storage_service/show_list.html.haml

  • ⚠️ - Line 1 - Layout/TrailingEmptyLines: Final newline missing.
  • ⚠️ - Line 1 - id attribute must be in lisp-case

@agrare agrare merged commit 9943399 into ManageIQ:master Feb 10, 2023
@Lauferin Lauferin mentioned this pull request Feb 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffibm jeffibm jeffibm left review comments

@agrare agrare agrare approved these changes

Assignees

@agrare agrare

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@OrGur1987 @jeffibm @agrare @miq-bot