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

Fixed massage emsrefresh completed successfully #228

Closed

Conversation

galoiring
Copy link
Contributor

currently there's no dedicated success message for propagating success from autosde backend to miq_task.
we only get an EmsRefresh success msg like:

a93b3af1-f7cb-4a64-ad4d-3b5b87161e8c

After the changes we made it looks like this :
Screenshot 2023-04-18 at 15 43 35

Related PR:

@galoiring galoiring changed the title fixed massage emsrefresh completed successfully Fixed massage emsrefresh completed successfully Apr 18, 2023
Comment on lines +94 to +96



Copy link
Member

Choose a reason for hiding this comment

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

Minor style, but can you trim these multiple blank lines to a single blank line?

@agrare
Copy link
Member

agrare commented Apr 20, 2023

It seems weird that the "action" would be "completed successfully", it does look like we could improve the task message though the targets aren't very helpful when they aren't class+id pairs.

Also this isn't autosde specific so if we're going to change the task probably should just be done here https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_refresh.rb#L169-L172

@galoiring
Copy link
Contributor Author

It seems weird that the "action" would be "completed successfully", it does look like we could improve the task message though the targets aren't very helpful when they aren't class+id pairs.

Also this isn't autosde specific so if we're going to change the task probably should just be done here https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_refresh.rb#L169-L172

If all the other providers does queue their task it will be fine.
whatever you decide

@agrare
Copy link
Member

agrare commented Apr 20, 2023

Yes all providers use this, lets focus on the core PR and see if we can clean up how we log the targets. I don't think we should say that the action is "EmsRefresh completed successfully " though since that doesn't make sense.

@miq-bot
Copy link
Member

miq-bot commented Apr 25, 2023

Checked commit Autosde@24a22ba with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
1 file checked, 4 offenses detected

app/models/manageiq/providers/autosde/storage_manager/ems_refresh_workflow.rb

@Fryguy Fryguy added the enhancement New feature or request label Apr 26, 2023
@miq-bot miq-bot added the stale label Jul 31, 2023
@miq-bot
Copy link
Member

miq-bot commented Jul 31, 2023

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@miq-bot
Copy link
Member

miq-bot commented Nov 6, 2023

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@miq-bot miq-bot closed this Nov 6, 2023
kbrock added a commit to kbrock/manageiq that referenced this pull request Nov 6, 2023
taken from:
- ManageIQ#22465
- ManageIQ/manageiq-providers-autosde#228

Instead of displaying target information, this now displays a user friendly message in the ui
@kbrock
Copy link
Member

kbrock commented Nov 6, 2023

alt: ManageIQ/manageiq#22776

kbrock added a commit to kbrock/manageiq that referenced this pull request Nov 7, 2023
taken from:
- ManageIQ#22465
- ManageIQ/manageiq-providers-autosde#228

Instead of displaying target information, this now displays a quick user friendly summary
kbrock added a commit to kbrock/manageiq that referenced this pull request Nov 14, 2023
taken from:
- ManageIQ#22465
- ManageIQ/manageiq-providers-autosde#228

Instead of displaying target information, this now displays a quick user friendly summary
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants