-
Notifications
You must be signed in to change notification settings - Fork 88
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
Add description field to the migration plan and add support for execution from specified step #235
base: main
Are you sure you want to change the base?
Conversation
bf84872
to
0b08007
Compare
Signed-off-by: Sergen Yalçın <[email protected]>
Signed-off-by: Sergen Yalçın <[email protected]>
b6636ba
to
aee87f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @sergenyalcin for introducing the step descriptions that will help folks give more context into what the migrator is doing and for what purpose.
We may also consider having a verbose mode for the plan executor, in which we will dump the description and the outputs from the executed steps. If the executor is not verbose, then we may consider just dumping the description strings.
And as also mentioned in the review comments, we can also consider giving some more context in the description texts, such the the provider names being installed/waited, files being applied, image URLs being pushed/replaced, etc. I believe this also depends whether we will have a mode of execution where we just dump the descriptions (an option for a cleaner output in my opinion). Let's discuss.
pkg/migration/testdata/plan/generated/configurationv1_pkg_migration_plan.yaml
Outdated
Show resolved
Hide resolved
@@ -46,6 +50,7 @@ spec: | |||
manualExecution: | |||
- "kubectl apply -f new-ssop/provider-family-aws.providers.pkg.crossplane.io_v1.yaml" | |||
type: Apply | |||
description: "Installing the new family provider" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: "Installing the new family provider" | |
description: "Installing the new family config provider" |
nit: We can also consider including the config provider's name in the description. This would require us to parse the references file in the manifest (and what about multiple manifests in a single file?) so we may do this if there's request for it. What do you think?
pkg/migration/testdata/plan/generated/configurationv1_pkg_migration_plan.yaml
Outdated
Show resolved
Hide resolved
pkg/migration/testdata/plan/generated/configurationv1_pkg_migration_plan.yaml
Outdated
Show resolved
Hide resolved
pkg/migration/testdata/plan/generated/configurationv1_pkg_migration_plan.yaml
Outdated
Show resolved
Hide resolved
pkg/migration/testdata/plan/generated/configurationv1_pkg_migration_plan.yaml
Outdated
Show resolved
Hide resolved
@@ -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++ { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
executors []Executor | ||
plan Plan | ||
callback ExecutorCallback | ||
LastSuccessfulStep int |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
Signed-off-by: Sergen Yalçın <[email protected]>
@ulucinar I made the changes you suggested about the wording in the descriptions. However, I would like to say a few things about the fact that it contains more information. It may be good to provide as much information as possible in the Description, but I think it is sufficient to set the summary information as a title in its current form. An example output is in this PR. (Of course, this is an older version, the wording has been improved.) |
Signed-off-by: Sergen Yalçın <[email protected]>
1045871
to
4a021e5
Compare
Description of your changes
This PR:
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested
Generated and reviewed migration plan