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

Check for VDDK conversion hosts before allowing warm migration #1082

Merged
merged 15 commits into from Jan 8, 2020

Conversation

@mturley
Copy link
Contributor

mturley commented Jan 3, 2020

On the Schedule step of the Migration Plan wizard, this PR replaces the warning icon (for VMs without snapshots) with two inline toast alerts, one for each of the potential reasons why warm migration might be unsupported. One or both will appear on mount of that step and disappear on unmount, if warm migration is blocked. The new alert appears if there are no VDDK-configured conversion hosts associated with the selected VMs.

In order to support more than one alert message at a time in the wizard, I changed the way we handle those alerts in Redux. The showAlertAction now takes an object of shape { alertText, alertType, alertId } (if no alertId is passed, all other alerts will be hidden and only the new one will show, matching the old behavior). The hideAlertAction now takes an optional alertId argument (if no alertId is passed, all alerts will be hidden). I made these changes only in the Plan wizard, but we also had the same alertText redux patterns duplicated in the mapping wizard, so if we ever need concurrent alerts there we can move this stuff to somewhere common.

Screenshot 2020-01-07 17 06 55

I updated/fixed the existing tests that were failing, but I'll add tests for the new code in a followup PR.

@mturley mturley requested a review from mzazrivec Jan 3, 2020
@mturley mturley added this to In progress in v2v UI via automation Jan 3, 2020
warmMigrationCompatibility: { isEveryVmCompatible, areConversionHostsConfigured },
showAlertAction
} = this.props;
if (!isEveryVmCompatible) {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Jan 3, 2020

Similar blocks of code found in 2 locations. Consider refactoring.

hideAlertAction('warmMigrationBadConversionHosts');
}

render() {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Jan 3, 2020

Function render has 67 lines of code (exceeds 25 allowed). Consider refactoring.

@@ -11,17 +11,32 @@ import { overwriteCsvConfirmModalProps } from '../../PlanWizardConstants';

export class PlanWizardVMStep extends React.Component {
componentDidMount() {
const { vm_choice_radio, editingPlan, shouldPrefillForEditing, queryPreselectedVmsAction, pristine } = this.props;
const {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Jan 3, 2020

Similar blocks of code found in 8 locations. Consider refactoring.

alertType: 'warning'
});
}
if (!areConversionHostsConfigured) {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Jan 3, 2020

Similar blocks of code found in 2 locations. Consider refactoring.

</Form>
);
class PlanWizardScheduleStep extends React.Component {
componentDidMount() {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Jan 3, 2020

Function componentDidMount has 26 lines of code (exceeds 25 allowed). Consider refactoring.

@miq-bot miq-bot added the wip label Jan 3, 2020
@mzazrivec

This comment has been minimized.

Copy link
Contributor

mzazrivec commented Jan 6, 2020

The current CI failures are related.

@mturley

This comment has been minimized.

Copy link
Contributor Author

mturley commented Jan 6, 2020

That's weird... prettier on my local env and on CI aren't agreeing. I must have screwed something up.

@mturley

This comment has been minimized.

Copy link
Contributor Author

mturley commented Jan 6, 2020

@mzazrivec are you able to run yarn test without failures on master? It fails for me...

@mturley

This comment has been minimized.

Copy link
Contributor Author

mturley commented Jan 6, 2020

I think it comes down to some version changes that happened when we removed yarn.lock in #1079, I guess prettier changed their rules. I'll make a PR to explicitly change back to the versions we were using..

@mzazrivec

This comment has been minimized.

Copy link
Contributor

mzazrivec commented Jan 7, 2020

@mturley Even with #1079 now in place, I see yarn lint failing (localy and on travis too).

@mzazrivec

This comment has been minimized.

Copy link
Contributor

mzazrivec commented Jan 7, 2020

@mturley after I run yarn lint:fix localy, yarn test still fails on my end, so 🤷 ...

@mturley mturley added ivanchuk/yes and removed hammer/yes labels Jan 7, 2020
@mturley mturley force-pushed the mturley:warm-migration-vddk-check branch from 914682f to be7fd05 Jan 7, 2020
network: 'Lan'
};

export const TRANSFORMATION_MAPPING_ITEM_DESTINATION_TYPES = {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Jan 7, 2020

Similar blocks of code found in 2 locations. Consider refactoring.

@mturley

This comment has been minimized.

Copy link
Contributor Author

mturley commented Jan 7, 2020

As for yarn lint failing, that must have been my editor using the newer prettier rules before we switched back in #1079, fixed. The other failures were because I hadn't run/fixed the tests yet, I've now fixed existing tests. I guess let's go ahead and get this merged and I'll add the new tests in another PR.

@mturley mturley force-pushed the mturley:warm-migration-vddk-check branch from be7fd05 to 51fb6cb Jan 7, 2020
@mturley mturley added the needs-tests label Jan 7, 2020
@mturley mturley changed the title [WIP] Check for VDDK conversion hosts before allowing warm migration Check for VDDK conversion hosts before allowing warm migration Jan 7, 2020
@mturley mturley removed the wip label Jan 7, 2020
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Jan 7, 2020

Checked commits mturley/manageiq-v2v@a505e8c~...51fb6cb with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🏆

@mzazrivec mzazrivec merged commit 32418d2 into ManageIQ:master Jan 8, 2020
2 of 3 checks passed
2 of 3 checks passed
codeclimate 8 issues to fix
Details
Hakiri No security warnings were found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
v2v UI automation moved this from In progress to Done Jan 8, 2020
@mturley mturley deleted the mturley:warm-migration-vddk-check branch Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v2v UI
  
Done
3 participants
You can’t perform that action at this time.