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

Ofer/326040 list capabilities for physical storage #8604

Merged
merged 1 commit into from
May 23, 2023
Merged

Ofer/326040 list capabilities for physical storage #8604

merged 1 commit into from
May 23, 2023

Conversation

Oferlis
Copy link

@Oferlis Oferlis commented Jan 18, 2023 •
edited

Added textual summary function to display capabilities of a physical storage.
related to PR in AutoSDE provider repo ManageIQ/manageiq-providers-autosde#198

Screenshot 2023-03-28 at 9 32 37

@Oferlis Oferlis requested a review from a team as a code owner January 18, 2023 10:52
@Oferlis Oferlis mentioned this pull request Jan 18, 2023
Merged
@jeffibm
Copy link
Member

jeffibm commented Jan 25, 2023

@Oferlis , could you please rebase with master (also squash 6 commits) branch to see if the the spec:javascript is still failing in GitHub actions

@Oferlis Oferlis force-pushed the ofer/326040-list_capabilities_for_physical_storage branch 2 times, most recently from 64f5040 to 61c35b1 Compare January 25, 2023 15:32
@Oferlis
Copy link
Author

Oferlis commented Jan 25, 2023

@Oferlis , could you please rebase with master (also squash 6 commits) branch to see if the the spec:javascript is still failing in GitHub actions

Hey, I have rebased the branch and fixed one of the failing specs, but the other keeps failing.

@jeffibm
Copy link
Member

jeffibm commented Jan 27, 2023

@Oferlis , could you please rebase with master (also squash 6 commits) branch to see if the the spec:javascript is still failing in GitHub actions

Hey, I have rebased the branch and fixed one of the failing specs, but the other keeps failing.

Could you try running them locally to see if this is related to your changes?

@Oferlis Oferlis force-pushed the ofer/326040-list_capabilities_for_physical_storage branch from 6574843 to ddbe5bd Compare January 29, 2023 09:54
@Oferlis
Copy link
Author

Oferlis commented Jan 31, 2023

Changing this PR to WIP since we decided that capabilities will be shown from the physical storage, not from the storage family

@Oferlis Oferlis changed the title Ofer/326040 list capabilities for physical storage WIP | Ofer/326040 list capabilities for physical storage Jan 31, 2023
@MelsHyrule
Copy link
Member

Manually adding WIP label (for the bot to add it automatically change the title to be [WIP] rather than WIP)

@MelsHyrule MelsHyrule added the wip label Feb 7, 2023
@Oferlis Oferlis changed the title WIP | Ofer/326040 list capabilities for physical storage [WIP] | Ofer/326040 list capabilities for physical storage Feb 8, 2023
agrare
agrare reviewed Feb 8, 2023
View reviewed changes
app/helpers/physical_storage_helper/textual_summary.rb Outdated
Comment on lines 107 to 109
if storage_family_id
capabilities = PhysicalStorageFamily.find(storage_family_id)&.capabilities
end
Copy link
Member

Choose a reason for hiding this comment

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

Two things:

  1. Prefer setting the physical_storage capabilities to be equal to the physical_storage_family.capabilities in your refresh parser so we only have to hit one location here to show them. Consider someone using the API where this logic won't exist (and shouldn't have to exist)
  2. If you were going to reference the storage_family for some reason you should use the assoication rather than doing a manual query (e.g. @record.physical_storage_family&.capabilities don't actually do this here, but if you had to in the future use the assoc not a manual query)

agrare
agrare reviewed Feb 8, 2023
View reviewed changes
spec/helpers/physical_storage_helper/textual_summary_spec.rb
Comment on lines +61 to +63
it 'shows 9 fields' do
expect(subject.items).to be_kind_of(Array)
expect(subject.items.size).to eq(8)
expect(subject.items.size).to eq(9)
Copy link
Member

Choose a reason for hiding this comment

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

Not a reflection on your changes at all, but this seems like a pointless test. Better to check the contents than a whole test checking the number of fields.

@Oferlis Oferlis force-pushed the ofer/326040-list_capabilities_for_physical_storage branch 2 times, most recently from 37d3b66 to 652794f Compare March 26, 2023 13:24
lint fix
18c7f12 
updated spec to contain capabilities

changed approach

linter errors fix

fallback added

Another approach

updated spec

fix lint errors

lint errors fixed

fix lint errors

modified capabilities parsing

fix

WIP-first attempt
@Oferlis Oferlis force-pushed the ofer/326040-list_capabilities_for_physical_storage branch from a088cce to 18c7f12 Compare March 28, 2023 06:32
@miq-bot
Copy link
Member

miq-bot commented Mar 28, 2023

Checked commit Autosde@18c7f12 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
2 files checked, 1 offense detected

app/helpers/physical_storage_helper/textual_summary.rb

@Oferlis Oferlis mentioned this pull request Mar 28, 2023
Merged
@Oferlis Oferlis changed the title [WIP] | Ofer/326040 list capabilities for physical storage Ofer/326040 list capabilities for physical storage Mar 28, 2023
@miq-bot miq-bot removed the wip label Mar 28, 2023
@jeffibm
Copy link
Member

jeffibm commented May 8, 2023

Hey @Oferlis , is this PR good to test/merge?

@jeffibm
Copy link
Member

jeffibm commented May 23, 2023

Hey @Oferlis , is this PR good to test/merge?

Hey, any updates here? or shall we merge it?

@Oferlis
Copy link
Author

Oferlis commented May 23, 2023

Hey, sorry for the late response.
You may merge it, thanks

@jeffibm jeffibm merged commit e032fb7 into ManageIQ:master May 23, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrare agrare agrare left review comments

@jeffibm jeffibm jeffibm 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
6 participants
@Oferlis @jeffibm @MelsHyrule @miq-bot @agrare @Fryguy