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

Add description field to the migration plan and add support for execution from specified step #235

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add description field to the migration plan and add support for execution from specified step #235

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c429b5
Add description field to the migration plan
sergenyalcin Jul 20, 2023
aee87f7
Support for executing plan from a specified step
sergenyalcin Jul 21, 2023
32ec719
Change the descriptions as suggested
sergenyalcin Jul 24, 2023
4a021e5
Protect the original value of skipDependencyResolution
sergenyalcin Jul 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Support for executing plan from a specified step 
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
  • Loading branch information
@sergenyalcin
sergenyalcin committed Jul 21, 2023
commit aee87f752339016470757121c41e8e308b6e7358
18 changes: 14 additions & 4 deletions pkg/migration/plan_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@ const (
// PlanExecutor drives the execution of a plan's steps and
// uses the configured `executors` to execute those steps.
type PlanExecutor struct {
executors []Executor
plan Plan
callback ExecutorCallback
executors []Executor
plan Plan
callback ExecutorCallback
LastSuccessfulStep int
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where do we read LastSuccessfulStep?

Copy link
Member Author

Choose a reason for hiding this comment

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

We read this value in the family-migrator. Please see this PR.

StartIndex int
}

// Action represents an action to be taken by the PlanExecutor.
Expand Down Expand Up @@ -69,6 +71,12 @@ func WithExecutorCallback(cb ExecutorCallback) PlanExecutorOption {
}
}

func WithStartIndex(startIndex int) PlanExecutorOption {
return func(pe *PlanExecutor) {
pe.StartIndex = startIndex
}
}

// ExecutorCallback is the interface for the callback implementations
// to be notified while executing each Step of a migration Plan.
type ExecutorCallback interface {
Expand Down Expand Up @@ -103,14 +111,15 @@ func NewPlanExecutor(plan Plan, executors []Executor, opts ...PlanExecutorOption

func (pe *PlanExecutor) Execute() error { //nolint:gocyclo // easier to follow this way
ctx := make(map[string]any)
for i := 0; i < len(pe.plan.Spec.Steps); i++ {
for i := pe.StartIndex; i < len(pe.plan.Spec.Steps); i++ {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have access to a logger in migration.PlanExecutor? If so, we may consider adding at least a debug statement that will convey the step index we are starting execution at.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have access to a logger. One option can be adding a log to callback implementation.

var r CallbackResult
if pe.callback != nil {
r = pe.callback.StepToExecute(pe.plan.Spec.Steps[i], i)
switch r.Action {
case ActionCancel:
return nil
case ActionSkip:
pe.LastSuccessfulStep = i
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do we use LastSucccessfulStep? If a step is skipped, is it still a successful one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I left that up to the reviews to talk about. In family-migrator, we return the skip action for the steps that the user has run manually. We should consider successful steps that are run manually and that the user validates it.

However, the skip action is a much more general action. Therefore, we can specify another type of action for such situations. (For manually performed steps)

Copy link
Member Author

@sergenyalcin sergenyalcin Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Or the another option can be that changing the name of the LastSucccessfulStep to LastPerformedStep.

This field is used for caching mechanism for plan. Please see this PR

I think, other approach can be moving this logic (storing the last performed step) to the migrator side.

continue
case ActionContinue, ActionRepeat:
}
Expand All @@ -124,6 +133,7 @@ func (pe *PlanExecutor) Execute() error { //nolint:gocyclo // easier to follow t
}
} else if pe.callback != nil {
r = pe.callback.StepSucceeded(pe.plan.Spec.Steps[i], i, diag)
pe.LastSuccessfulStep = i
}

switch r.Action {
Expand Down
2 changes: 1 addition & 1 deletion pkg/migration/plan_generator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ func TestGeneratePlan(t *testing.T) {
if err != nil {
t.Fatalf("Failed to load plan file from path %s: %v", tt.want.migrationPlanPath, err)
}
if diff := cmp.Diff(p, &pg.Plan, cmpopts.IgnoreUnexported(Spec{})); diff != "" {
if diff := cmp.Diff(p, &pg.Plan, cmpopts.IgnoreUnexported(Plan{}, Spec{})); diff != "" {
t.Errorf("GeneratePlan(): -wantPlan, +gotPlan: %s", diff)
}
// compare generated migration files with the expected ones
Expand Down