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 edit mode: check which attached resources are compliant to selected capabilities #8750

Merged
merged 1 commit into from
May 10, 2023
Merged

storage service edit mode: check which attached resources are compliant to selected capabilities #8750

merged 1 commit into from
May 10, 2023

Conversation

OrGur1987
Copy link
Contributor

@OrGur1987 OrGur1987 commented Apr 10, 2023 •
edited

issue: #8749

added a button in service edit mode which enables the user to check what currently attached resources will comply with the selected capabilities.

this includes a new async-action-button.jsx which is based on async-credentials.jsx, and enables sending an async request to the provider's backend and display the resolved results (or an error message if needed).

if there's a way to beautify the returning string, or even render it in a table, it could be a great improvement.

the enhancement is based on the yet to be merged #8728, and should be rebased on master after that issue is merged.

Screen.Recording.2023-04-10.at.13.40.27.mov

@OrGur1987 OrGur1987 requested a review from a team as a code owner April 10, 2023 10:51
@OrGur1987 OrGur1987 mentioned this pull request Apr 10, 2023
Closed
5 tasks
@OrGur1987 OrGur1987 force-pushed the add_resource_compliance_check_to_service_update branch from 5fe4e63 to 4e5f88d Compare April 10, 2023 12:21
@agrare agrare self-assigned this Apr 12, 2023
@OrGur1987
Copy link
Contributor Author

@jeffibm can we ignore this?
#8750 (comment)

@jeffibm
Copy link
Member

jeffibm commented Apr 17, 2023

@jeffibm can we ignore this?
#8750 (comment)

yea, no worries!

@jeffibm
Copy link
Member

jeffibm commented Apr 17, 2023

same here, when we hit the check compliant resources button, we get this notification. could you put them in a flash message which looks better!
image

@OrGur1987
Copy link
Contributor Author

OrGur1987 commented Apr 17, 2023 •
edited

same here, when we hit the check compliant resources button, we get this notification. could you put them in a flash message which looks better!
image

but are we supposed to get this? or are u talking about error handling in general?

I believe u get this because u need to use the branches from all the PRs listed in the issue:

@jeffibm
Copy link
Member

jeffibm commented Apr 17, 2023 •
edited

same here, when we hit the check compliant resources button, we get this notification. could you put them in a flash message which looks better!
image

but are we supposed to get this? or are u talking about error handling in general?

I believe u get this because u need to use the branches from all the PRs listed in the issue:

ohh..I did see those dependent PR's. could you correct those links. it all points to the issue in ui-classic repo now :)

@jeffibm
Copy link
Member

jeffibm commented Apr 17, 2023 •
edited

yes, handling errors in general

but are we supposed to get this? or are u talking about error handling in general?

@OrGur1987
Copy link
Contributor Author

but are we supposed to get this? or are u talking about error handling in general?

yes, handling errors in general

ok, I'll look into that, haven't dealt with these flash messages before. could u point me to an example?

links to the PRs (they just have the same name as the issue, but each leads to a PR in a different repo):

@jeffibm
Copy link
Member

jeffibm commented Apr 17, 2023

I haven't checked yet. can these messages be handled (or say console.log) in react components? if yes, it can be done like
add_flash('error', __('Invalid credentials'));

@OrGur1987 OrGur1987 force-pushed the add_resource_compliance_check_to_service_update branch from 4e5f88d to 0130b5f Compare April 17, 2023 11:27
@OrGur1987
Copy link
Contributor Author

I haven't checked yet. can these messages be handled (or say console.log) in react components? if yes, it can be done like
add_flash('error', __('Invalid credentials'));

to be honest it sounds to me like a more fundamental change in error handling, not something particular to this PR or specific model

@OrGur1987 OrGur1987 force-pushed the add_resource_compliance_check_to_service_update branch from 0130b5f to f8daf57 Compare April 17, 2023 18:31
@OrGur1987 OrGur1987 force-pushed the add_resource_compliance_check_to_service_update branch 2 times, most recently from 8ea962c to e041457 Compare April 24, 2023 09:23
@OrGur1987
Copy link
Contributor Author

I haven't checked yet. can these messages be handled (or say console.log) in react components? if yes, it can be done like
add_flash('error', __('Invalid credentials'));

to be honest it sounds to me like a more fundamental change in error handling, not something particular to this PR or specific model

@jeffibm
hi, can we continue with this PR as it is or do you prefer solving the general error handling/presentation issue?
thanks

@jeffibm
Copy link
Member

jeffibm commented May 2, 2023

yeah, we can continue with this PR. but please rebase with the master to fix the failing specs...

@OrGur1987 OrGur1987 force-pushed the add_resource_compliance_check_to_service_update branch from e041457 to 015a860 Compare May 2, 2023 10:47
@OrGur1987
Copy link
Contributor Author

yeah, we can continue with this PR. but please rebase with the master to fix the failing specs...

@jeffibm
do u know why the WhiteSource Security Check fails?

@jeffibm
Copy link
Member

jeffibm commented May 2, 2023

yeah, we can continue with this PR. but please rebase with the master to fix the failing specs...

@jeffibm do u know why the WhiteSource Security Check fails?

No, but I think we can ignore them for now..

jeffibm
jeffibm reviewed May 4, 2023
View reviewed changes
app/javascript/components/storage-service-form/get-compliant-resources.jsx Outdated
@@ -0,0 +1,64 @@
import React from 'react';
import { pick } from 'lodash';
// import AsyncCredentials from '../async-credentials/async-credentials';
Copy link
Member

Choose a reason for hiding this comment

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

please remove this line if not needed

@jeffibm
Copy link
Member

jeffibm commented May 4, 2023

Hey @agrare, can I merge this?

@OrGur1987
added button in service edit mode which enables user to check what cu…
1dce268 
…rrently attached resources will comply with the selected capabilities.
@OrGur1987 OrGur1987 force-pushed the add_resource_compliance_check_to_service_update branch from 015a860 to 1dce268 Compare May 4, 2023 06:04
@miq-bot
Copy link
Member

miq-bot commented May 4, 2023

Checked commit Autosde@1dce268 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@jeffibm jeffibm merged commit 4080fec into ManageIQ:master May 10, 2023
8 of 9 checks passed
@OrGur1987 OrGur1987 mentioned this pull request Jun 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffibm jeffibm jeffibm approved these changes

Assignees

@agrare agrare

@jeffibm jeffibm

Labels
enhancement storage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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