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

Schedule migration modal changes for warm migration #1070

Open

Conversation

@mzazrivec
Copy link
Contributor

mzazrivec commented Nov 25, 2019

Schedule & Migrate buttons got merged into one
warm-scheduling-01

Schedule migration now
warm-scheduling-02

Schedule migration later
warm-scheduling-03

Schedule retry button
warm-scheduling-04

Schedule retry modal
warm-scheduling-05

Schedule cutover button
warm-scheduling-06

Schedule cutover modal
warm-scheduling-07

)}
</Spinner>
</Grid.Col>
<ScheduleMigrationModal

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Nov 25, 2019

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

startMigrationNowHandler(!this.state.showDatepicker);
}

componentDidUpdate(prevProps, prevState, snapshot) {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Nov 25, 2019

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

startMigrationNowHandler(!this.state.showDatepicker);
}

componentDidUpdate(prevProps, prevState, snapshot) {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Nov 25, 2019

Function componentDidUpdate has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.

{__('Schedule')}
</Button>
);
const ScheduleMigrationButton = ({ loading, toggleScheduleMigrationModal, plan, isMissingMapping }) => {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Nov 25, 2019

Function ScheduleMigrationButton has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.

@miq-bot miq-bot added the wip label Nov 25, 2019
@mzazrivec mzazrivec force-pushed the mzazrivec:schedule_migration_modal_changes_for_warm_migration branch from 779944f to 8cba3f9 Nov 27, 2019
@mturley mturley added this to In progress in v2v UI via automation Dec 6, 2019
@mzazrivec mzazrivec force-pushed the mzazrivec:schedule_migration_modal_changes_for_warm_migration branch 2 times, most recently from df322f8 to 310357d Dec 13, 2019
)}
</Spinner>
</Grid.Col>
<ScheduleMigrationModal

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Dec 13, 2019

Identical blocks of code found in 3 locations. Consider refactoring.

@mturley

This comment has been minimized.

Copy link
Contributor

mturley commented Jan 2, 2020

Hey @mzazrivec, I hope you enjoyed your holidays.

When you have a chance I think we should figure out how to get this and #1077 merged, I'm sure there will be conflicts and I think it'll be a mess if we try to resolve them at the last minute when the backend is ready. Considering that warm migration is already "broken" on master (you can select it in the wizard and it doesn't work properly) I think maybe we can justify merging this stuff before we test it with the backend, and then consider any issues that come up as separate bugs to be fixed when the backend is testable... does this sound reasonable or do you think we should instead merge them with flags in place to disable the new behavior? (or leave them open)

@mzazrivec

This comment has been minimized.

Copy link
Contributor Author

mzazrivec commented Jan 6, 2020

@mturley I'd say we should try to merge our work now and resolve whatever comes up when needed. Enable / disable flags for this feature? Probably not. It would just make things more complicated.

I still need to address the tests here before this PR can be merged.

@mturley

This comment has been minimized.

Copy link
Contributor

mturley commented Jan 6, 2020

@mzazrivec Ok sounds good. It sounds like Fabien and Martin are hoping to have the warm migration backend changes done this week and then want to get all the UI merged for the build on Monday... I'm hoping they leave us a day or two to test everything end to end but it might be a crunch 😞 I also need to finish writing unit tests for #1077 and #1082. I was thinking about asking if you are ok with merging this stuff without tests and then adding tests in another PR, but if we can get the conflicts resolved soon I guess that's not necessary. Thanks

@mzazrivec

This comment has been minimized.

Copy link
Contributor Author

mzazrivec commented Jan 7, 2020

@mturley I'm quite OK with merging the PRs without unit tests as long as the existing tests pass.

@mzazrivec mzazrivec force-pushed the mzazrivec:schedule_migration_modal_changes_for_warm_migration branch 2 times, most recently from 3ae6d95 to 58e1dc7 Jan 8, 2020
@@ -162,6 +167,16 @@ const MigrationInProgressListItem = ({
{sprintf(__('Running playbook service %s. This might take a few minutes.'), playbookName)}
</ListViewTable.InfoItem>
]}
actions={

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Jan 8, 2020

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

@@ -197,6 +212,16 @@ const MigrationInProgressListItem = ({
</div>
</ListViewTable.InfoItem>
]}
actions={

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Jan 8, 2020

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

@mzazrivec mzazrivec force-pushed the mzazrivec:schedule_migration_modal_changes_for_warm_migration branch 2 times, most recently from 9fde49f to 7eb0e9d Jan 8, 2020
@mzazrivec mzazrivec changed the title [WIP] Schedule migration modal changes for warm migration Schedule migration modal changes for warm migration Jan 8, 2020
@mzazrivec mzazrivec removed the wip label Jan 8, 2020
@mzazrivec

This comment has been minimized.

Copy link
Contributor Author

mzazrivec commented Jan 8, 2020

@mturley I don't have tests for this PR in place (can add in a separate PR), but I think it's ready for review.

@mzazrivec mzazrivec requested a review from mturley Jan 8, 2020
@mturley

This comment has been minimized.

Copy link
Contributor

mturley commented Jan 8, 2020

Sounds good @mzazrivec, I think we can get as much merged for Monday's build as possible and then follow up with unit tests. I made a new label needs-tests to keep track of which PRs we need to return to.

@mturley mturley added the needs-tests label Jan 8, 2020
@mturley

This comment has been minimized.

Copy link
Contributor

mturley commented Jan 8, 2020

Well, I guess Monday's build of ivanchuk won't have any of this stuff until it's all ready, so that might not matter depending on the backend.

Copy link
Contributor

mturley left a comment

It's tough to test this stuff without real data, I'm sorry about that. There are a few issues I think we'll run into though. Otherwise this looks good.

@@ -162,6 +165,16 @@ const MigrationInProgressListItem = ({
{sprintf(__('Running playbook service %s. This might take a few minutes.'), playbookName)}
</ListViewTable.InfoItem>
]}
actions={
<div>
<ScheduleMigrationButton

This comment has been minimized.

Copy link
@mturley

mturley Jan 8, 2020

Contributor

The Schedule Migration button is appearing in the in-progress view even for cold migrations. We need to make sure it only appears on an in-progress plan if it's a warm migration plan and it's not already past its scheduled cutover. Also I think we need to make it so that after a cutover is scheduled, the schedule button doesn't appear and instead we have an Edit Schedule item in a kebab menu, like we do for cold migrations.

@@ -197,6 +210,16 @@ const MigrationInProgressListItem = ({
</div>
</ListViewTable.InfoItem>
]}
actions={
<div>
<ScheduleMigrationButton

This comment has been minimized.

Copy link
@mturley

mturley Jan 8, 2020

Contributor

Same thing here

super(props);

const { scheduleMigrationPlan } = this.props;
const { migrationScheduled } = getPlanScheduleInfo(scheduleMigrationPlan) || '';

This comment has been minimized.

Copy link
@mturley

mturley Jan 8, 2020

Contributor

I think we need to look at the getPlanScheduleInfo helper and make sure it is aware of warm migrations, because currently the migrationScheduled value here will always be the cold-migration scheduled time. So if the user already scheduled a warm migration cutover and they open the modal again to edit that schedule, the state won't pre-populate with their cutover time.

super(props);

this.state = {
showDatepicker: false

This comment has been minimized.

Copy link
@mturley

mturley Jan 8, 2020

Contributor

This showDatepicker state value seems like it's just duplicating the startMigrationNow value from the parent component, since you're having to keep them in sync. It might be better to just have showDatepicker be a prop, and pass in showDatepicker={!startMigrationNow} from the parent so there's a single source of truth for that.

That way, in componentDidUpdate you can just detect the prop changing and use that to call the datetimepicker init stuff, and you won't have to call the startMigrationNowHandler in there or in componentDidMount. You can instead just call that handler in your handleRadioChange callback to update the lifted state.

Also startMigrationNowHandler is a bit of a confusing name, at first I thought calling it would start the migration. Maybe setScheduleMode or something like that would be more clear (I don't feel strongly about that though).

This comment has been minimized.

Copy link
@mturley

mturley Jan 8, 2020

Contributor

I guess you'd still have to make sure you call the datepicker init stuff in componentDidMount if startMigrationNow is already false when this mounts, but you can probably do that by moving that stuff to a helper method that you call in both componentDidMount and componentDidUpdate.

@mzazrivec mzazrivec force-pushed the mzazrivec:schedule_migration_modal_changes_for_warm_migration branch from 7eb0e9d to 19e7d34 Jan 14, 2020
@mzazrivec mzazrivec force-pushed the mzazrivec:schedule_migration_modal_changes_for_warm_migration branch from 19e7d34 to 475c473 Jan 17, 2020
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Jan 17, 2020

Checked commits mzazrivec/miq_v2v_ui_plugin@79c3867~...475c473 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. ⭐️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v2v UI
  
In progress
3 participants
You can’t perform that action at this time.