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

Feature/ret 1756 create migrate command #121

Merged
merged 2 commits into from
Aug 29, 2019
Merged

Feature/ret 1756 create migrate command #121

merged 2 commits into from
Aug 29, 2019

Conversation

ivonx
Copy link
Contributor

@ivonx ivonx commented Aug 11, 2019

No description provided.

@ivonx ivonx requested review from martin-v and modulo11 August 11, 2019 19:01
@cla-bot cla-bot bot added the cla-signed label Aug 11, 2019
@ivonx ivonx force-pushed the feature/RET-1756-create-migrate-command branch 3 times, most recently from fc2b6a2 to 00d2a33 Compare August 11, 2019 19:30
@rebazer rebazer force-pushed the feature/RET-1756-create-migrate-command branch from 00d2a33 to 9438529 Compare August 12, 2019 06:37
@ivonx ivonx force-pushed the feature/RET-1756-create-migrate-command branch from 9438529 to 9841340 Compare August 12, 2019 08:00
@rebazer rebazer force-pushed the feature/RET-1756-create-migrate-command branch from 9841340 to 648d8c4 Compare August 12, 2019 09:15
@ivonx ivonx force-pushed the feature/RET-1756-create-migrate-command branch 13 times, most recently from 88298d5 to 7b6077e Compare August 14, 2019 12:25
@rebazer rebazer force-pushed the feature/RET-1756-create-migrate-command branch 2 times, most recently from 220dd39 to 8a7b510 Compare August 21, 2019 09:16
@beatngu13
Copy link
Contributor

I suggest to close this PR for two reasons:

  1. It is untested, no unit or integration tests.
  2. We need the same functionality in review. How are (paying) customers supposed to migrate their GMs? Switch to the CLI?

It's probably better to have the core functionality in recheck and then integrate it properly in both frontends.

@beatngu13 beatngu13 mentioned this pull request Aug 23, 2019
Open
@rebazer rebazer force-pushed the feature/RET-1756-create-migrate-command branch from 8a7b510 to 0445f91 Compare August 23, 2019 23:13
@rebazer rebazer force-pushed the feature/RET-1756-create-migrate-command branch from 0445f91 to c06568f Compare August 27, 2019 07:00
@martin-v
Copy link
Member 

2. We need the same functionality in review. How are (paying) customers supposed to migrate their GMs? Switch to the CLI?

It's probably better to have the core functionality in recheck and then integrate it properly in both frontends.

The functionality is in recheck's persistence module, this is only a mapper for cli.

@rebazer rebazer merged commit 9585190 into master Aug 29, 2019
@rebazer rebazer deleted the feature/RET-1756-create-migrate-command branch August 29, 2019 09:12
@beatngu13
Copy link
Contributor

Yet, the entire migrate CLI command is untested … we usually require integration tests before merge (see e.g. CommitIT, DiffIT, IgnoreIT).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martin-v martin-v martin-v approved these changes

@modulo11 modulo11 Awaiting requested review from modulo11

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ivonx @beatngu13 @martin-v @rebazer