Lifecycle overrides #4445
Lifecycle overrides #4445
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/assign @justinsb I think this is more your style. It is not wired into the cli yet, and apply_cluster.go needs some tweaks, but PTAL |
upup/pkg/fi/task.go
Outdated
| } | ||
| hl, ok := task.(HasLifecycle) | ||
| if !ok { | ||
| glog.Fatalf("task %T does not implement HasLifecycle", task) |
There was a problem hiding this comment.
This needs to be a warning and a return. Problem is with nodeup tasks. This is breaking CI
k8s-ci-robot
added
the
do-not-merge/work-in-progress
chrislovecnm
force-pushed
the
lifecycle-overrides
branch
3 times, most recently
from
cdbda11
to
7217017
Compare
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
| @@ -109,6 +113,8 @@ func NewCmdUpdateCluster(f *util.Factory, out io.Writer) *cobra.Command { | |||
| cmd.Flags().StringVar(&options.OutDir, "out", options.OutDir, "Path to write any local output") | |||
| cmd.Flags().BoolVar(&options.CreateKubecfg, "create-kube-config", options.CreateKubecfg, "Will control automatically creating the kube config file on your local filesystem") | |||
| cmd.Flags().StringVar(&options.Phase, "phase", options.Phase, "Subset of tasks to run: "+strings.Join(cloudup.Phases.List(), ", ")) | |||
| cmd.Flags().StringSliceVar(&options.LifecycleOverrides, "lifecycle-overrides", options.LifecycleOverrides, "comma separated list of phase overrides, example: SecurityGroups=Ignore,InternetGateway=ExistsAndWarnIfChanges") | |||
There was a problem hiding this comment.
I separated this out so that a phase does not have to be set. If we include this with the --phase flag a user would not be able to run all of the phases. Thoughts?
There was a problem hiding this comment.
I am open to other ideas, but I do not want to force a user to define a phase in order to use overrides. Users will want to use overrides but not specify a phase. We can do phase="", but that is ugly and a change to syntax. Phases are a group of tasks and lifecycles, while lifecycle overrides are just setting the lifecycle on a single task. Kinda stumped how to do this well.
@rdrgmnzs I know you are going to use this functionality, any opinion?
|
@justinsb PTAL. I think I am going to close my other lifecycle PR because I like this one better. Let me know what you think! |
chrislovecnm
force-pushed
the
lifecycle-overrides
branch
from
7217017
to
64439b4
Compare
|
I was planning on adding testing to my SecurityGroup PR for this functionality. Any ideas on easy unit testing? /assign @rdrgmnzs |
| return fmt.Errorf("Incorrect syntax for lifecyle-overrides, correct syntax is TaskName=lifecycleName, override provided: %q", override) | ||
| } | ||
|
|
||
| taskName := values[0] |
There was a problem hiding this comment.
This is going to be case-sensitive (and fail silently on case mismatch)
| if v, ok := fi.LifecycleNameMap[lifecycle]; ok { | ||
| return v, nil | ||
| } | ||
| return "", fmt.Errorf("unknown lifecycle %q, available lifecycle: %s", lifecycle, strings.Join(fi.Lifecycles.List(), ",")) |
There was a problem hiding this comment.
(This one is fine, because we fail-fast and give a helpful message)
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrislovecnm, justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR adds the capability to override a task lifecycle dynamically. A list of lifecycles keyed by task names is provided, and those lifecycles can override a matching task name.