Add description field to the migration plan and add support for execution from specified step#235
Add description field to the migration plan and add support for execution from specified step#235sergenyalcin wants to merge 4 commits intocrossplane:mainfrom
Conversation
bf84872 to
0b08007
Compare
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
b6636ba to
aee87f7
Compare
ulucinar
left a comment
There was a problem hiding this comment.
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
| 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.
| 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
| 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.
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.
We don't have access to a logger. One option can be adding a log to callback implementation.
| case ActionCancel: | ||
| return nil | ||
| case ActionSkip: | ||
| pe.LastSuccessfulStep = i |
There was a problem hiding this comment.
How do we use LastSucccessfulStep? If a step is skipped, is it still a successful one?
There was a problem hiding this comment.
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.
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.
Where do we read LastSuccessfulStep?
There was a problem hiding this comment.
We read this value in the family-migrator. Please see this PR.
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
|
@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 <yalcinsergen97@gmail.com>
1045871 to
4a021e5
Compare
Description of your changes
This PR:
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
Generated and reviewed migration plan