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

[#400] Plan details playbooks support #442

Merged
merged 1 commit into from
Jul 24, 2018

Conversation

michaelkro
Copy link
Contributor

@michaelkro michaelkro commented Jun 26, 2018

fixes #400

Notes

  1. Will squash all the WIP commits after review (reason: see 2) Done
  2. One way to test this locally is to reset --hard HEAD^^. That will undo a cleanup commit and the commit that removes the mocked data/API responses. Using the nightly db (or any db you have that has in progress plans), go to Plans in Progress and click on any plan. Things to check
    • Download Log dropdowns
    • Success notifications after "downloading" a playbook log (pre or post)
    • Info Popovers~
  3. Will need help verifying playbook log download implementation
  4. Should be merged with or after Add support for pre/post-migration playbook manageiq-content#355

Screens

Running Post-migration Playbook

fullscreen_7_9_18__5_31_pm

fullscreen_7_9_18__5_25_pm-2

No Playbooks

fullscreen_7_9_18__5_27_pm

fullscreen_7_9_18__5_28_pm

Running Pre-migration Playbook

fullscreen_7_9_18__5_28_pm

fullscreen_7_9_18__5_29_pm

Playbook Log Download Success

fullscreen_7_15_18__2_47_pm

Playbook Log Download Failure

fullscreen_7_15_18__3_04_pm

https://bugzilla.redhat.com/show_bug.cgi?id=1564250

@michaelkro michaelkro force-pushed the playbooks-plan-details branch 14 times, most recently from 61848e7 to 8135419 Compare July 2, 2018 16:11
}}
<DropdownButton
id={`${task.id}-${task.descriptionPrefix}_download_log_dropdown`}
title="Download Log"
Copy link
Member

Choose a reason for hiding this comment

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

__('Download Log')

{__('Download Log')}
</a>
{task.options.showPreMigrationOption && (
<MenuItem eventKey="preMigration">Pre-migration log</MenuItem>
Copy link
Member

Choose a reason for hiding this comment

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

more 18n "Pre-migration" / "Post-migration"

@michaelkro michaelkro force-pushed the playbooks-plan-details branch 7 times, most recently from 517c077 to def1e45 Compare July 11, 2018 14:41
@michaelkro michaelkro force-pushed the playbooks-plan-details branch from 7676f71 to bca7de1 Compare July 12, 2018 17:38
dispatch({
type: V2V_NOTIFICATION_ADD,
message: successMsg,
notificationType: 'success',
Copy link
Member

Choose a reason for hiding this comment

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

nit... later on - we can move notificationType to a constant...


if (hasPlaybookService) {
taskDetails.options.prePlaybookRunning =
task.options.playbooks.pre && task.options.playbooks.pre.job_state === 'active';
Copy link
Member

Choose a reason for hiding this comment

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

we use PLAN_JOB_STATE constants later on - not a blocker at all right now

Copy link
Member

@priley86 priley86 left a comment

Choose a reason for hiding this comment

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

Preliminary checks are all good here...

Would be nice to merge and get QE feedback.

@michaelkro michaelkro force-pushed the playbooks-plan-details branch from f91dac0 to 2fe745f Compare July 20, 2018 11:19
Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

😋 👍 mostly majorly ok with this, just a few nits and questions 🙇‍♀️ 🌄

itzah wonderful addition! 🤗

type: DOWNLOAD_LOG_COMPLETED,
payload: task.id
});
dispatch({
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe we need to add this notification, as this is a catch on an API.get miq notification system should take over signaling the error should any arise...

@@ -57,6 +67,20 @@ const processVMTasks = vmTasks => {
options: {}
};

const hasPlaybookService = task.options.playbooks;

if (hasPlaybookService) {
Copy link
Member

Choose a reason for hiding this comment

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

super minor nit, but we don't use hasPlaybookService anywhere but here, why not save a line, just do task.options.playbooks here?

overlayTriggerClick = task => {
if (task.options.playbooks) {
const playbookStatuses = task.options.playbooks;
let runningPlaybook;
Copy link
Member

Choose a reason for hiding this comment

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

another nit... any chance we can set this = null? I know I know, undefined will also be evaluated to false on line 258.... 😬 (if not 'saul good 🤗)

e.preventDefault();
downloadLogAction(task);
}}
<DropdownButton
Copy link
Member

Choose a reason for hiding this comment

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

Ok quick question.. I have no idea how this worked before... but If a user doesn't use a pre/post migration playbook, will they still have a log to download here? If so... maybe there should be a third option shown... to download that...

(before we had playbook support there were logs to download, just want to make sure we don't lose those)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, you're totally correct. The regular Migration Log option is down here line 494

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which, apparently I didn't translate

Copy link
Member

Choose a reason for hiding this comment

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

ahh see thats what I meant! 😏 🤣

Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

Yer a 👨‍🎤 @michaelkro, this is wonderful 🤗 💃

* Dropdown button for log downloads
* Pre/post playbook logs will be obtained from the stdout attribute on
  their associated OrchestrationStack
* Update task info popover to indicate when a pre/post playbook is
  running
* Fetch running playbook template in order to display name in popover
@michaelkro michaelkro force-pushed the playbooks-plan-details branch from 039b918 to 3d3fe1d Compare July 20, 2018 20:16
@AparnaKarve
Copy link
Contributor

Looks good!

Will be testing this some more on the appliance today.

@AparnaKarve AparnaKarve merged commit 18e320d into ManageIQ:master Jul 24, 2018
michaelkro added a commit to michaelkro/manageiq-v2v that referenced this pull request Jul 30, 2018
michaelkro added a commit to michaelkro/manageiq-v2v that referenced this pull request Jul 31, 2018
simaishi pushed a commit that referenced this pull request Jul 31, 2018
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 6d8758f2124b69374c256b42ab60187951a0966b
Author: Aparna Karve <[email protected]>
Date:   Tue Jul 24 12:35:08 2018 -0700

    Merge pull request #442 from michaelkro/playbooks-plan-details
    
    [#400] Plan details playbooks support
    (cherry picked from commit 18e320d73da6da62b5ccf316d1ca0bb666c02790)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1608420

@simaishi
Copy link
Contributor

@michaelkro @AparnaKarve Travis is now failing in Gaprindashvili branch. I think it's caused by this PR. Can you please take a look?

/home/travis/build/ManageIQ/miq_v2v_ui_plugin/app/javascript/react/screens/App/Plan/components/PlanRequestDetailList/PlanRequestDetailList.js
  212:3  error  renderInput should be placed after overlayTriggerClick  react/sort-comp
  238:3  error  onSelect should be placed before setPage                react/sort-comp

https://travis-ci.org/ManageIQ/miq_v2v_ui_plugin/jobs/410122179

@michaelkro
Copy link
Contributor Author

michaelkro commented Jul 31, 2018

@simaishi @AparnaKarve I've put up a PR against G to fix the errors #524

@michaelkro michaelkro deleted the playbooks-plan-details branch August 2, 2018 12:48
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.

Pre & Post Playbook Support - Plan Details Page Changes
5 participants