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

kubeadm add phase runner #69684

Merged
merged 1 commit into from Oct 12, 2018

Conversation

@fabriziopandini
Contributor

fabriziopandini commented Oct 11, 2018

What this PR does / why we need it:
This PR implements the Phase runner as defined in KEP0029

Which issue(s) this PR fixes :
Rif kubernetes/kubeadm#1163

Special notes for your reviewer:

/sig cluster-lifecycle
/kind documentation
/assign @neolit123
/cc @timothysc
@kubernetes/sig-cluster-lifecycle-pr-reviews
@chuckha
@rosti

Release note:

NONE
@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123

neolit123 Oct 11, 2018

Member

thanks, best to rename the title as there are now 2 PRs kubeadm add phase runner

Member

neolit123 commented Oct 11, 2018

thanks, best to rename the title as there are now 2 PRs kubeadm add phase runner

@timothysc

So I'm a little concerned we're over-engineering for what we need at this point, and could leverage existing libraries to some of the internal logistics.

Show outdated Hide outdated cmd/kubeadm/app/cmd/phases/workflow/doc.go Outdated
Show outdated Hide outdated cmd/kubeadm/app/cmd/phases/workflow/doc_test.go Outdated
Show outdated Hide outdated cmd/kubeadm/app/cmd/phases/workflow/phase.go Outdated
Show outdated Hide outdated cmd/kubeadm/app/cmd/phases/workflow/phase.go Outdated
Show outdated Hide outdated cmd/kubeadm/app/cmd/phases/workflow/phase.go Outdated
Show outdated Hide outdated cmd/kubeadm/app/cmd/phases/workflow/phase.go Outdated
Show outdated Hide outdated cmd/kubeadm/app/cmd/phases/workflow/phase.go Outdated
@@ -0,0 +1,50 @@
/*

This comment has been minimized.

@timothysc

timothysc Oct 11, 2018

Member

I'd typically call this interface.go. That's the standard pattern in k8s.

@timothysc

timothysc Oct 11, 2018

Member

I'd typically call this interface.go. That's the standard pattern in k8s.

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 12, 2018

Contributor

In order to simplify I removed the interface and now there is only a struct

@fabriziopandini

fabriziopandini Oct 12, 2018

Contributor

In order to simplify I removed the interface and now there is only a struct

Show outdated Hide outdated cmd/kubeadm/app/cmd/phases/workflow/runner.go Outdated
Phase
// provide access to the parent phase in the workflow managed by the Runner.
parent *phaseRunner

This comment has been minimized.

@timothysc

timothysc Oct 11, 2018

Member

There are a lot of pre-existing dag or tree libraries that exist. We need to be sure we are scoping the usage correctly and only building what we need.

@timothysc

timothysc Oct 11, 2018

Member

There are a lot of pre-existing dag or tree libraries that exist. We need to be sure we are scoping the usage correctly and only building what we need.

This comment has been minimized.

@timothysc

timothysc Oct 12, 2018

Member

Could you put a //TODO: If we ever decide to get more sophisticated we can swap this type with a well defined dag or tree library.

@timothysc

timothysc Oct 12, 2018

Member

Could you put a //TODO: If we ever decide to get more sophisticated we can swap this type with a well defined dag or tree library.

@timothysc timothysc referenced this pull request Oct 11, 2018

Merged

Kubeadm graduate preflight phase #69666

1 of 1 task complete
@fabriziopandini

This comment has been minimized.

Show comment
Hide comment
@fabriziopandini

fabriziopandini Oct 12, 2018

Contributor

@timothysc I further simplified things removing the interface; let me know if now it is better

Contributor

fabriziopandini commented Oct 12, 2018

@timothysc I further simplified things removing the interface; let me know if now it is better

@timothysc

/approve

minor comments then lgtm and lets keep moving.

Show outdated Hide outdated cmd/kubeadm/app/cmd/phases/workflow/doc_test.go Outdated
Phase
// provide access to the parent phase in the workflow managed by the Runner.
parent *phaseRunner

This comment has been minimized.

@timothysc

timothysc Oct 12, 2018

Member

Could you put a //TODO: If we ever decide to get more sophisticated we can swap this type with a well defined dag or tree library.

@timothysc

timothysc Oct 12, 2018

Member

Could you put a //TODO: If we ever decide to get more sophisticated we can swap this type with a well defined dag or tree library.

Example string
// Hidden define if the phase should be hidden in the workflow help.
Hidden bool

This comment has been minimized.

@timothysc

timothysc Oct 12, 2018

Member

Could you outline cases here for possible phases that "should" be hidden?

e.g. - WaitForStart

@timothysc

timothysc Oct 12, 2018

Member

Could you outline cases here for possible phases that "should" be hidden?

e.g. - WaitForStart

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 12, 2018

Contributor

done

@fabriziopandini

fabriziopandini Oct 12, 2018

Contributor

done

// BindToCommand bind the Runner to a cobra command by altering
// command help, adding phase related flags and by adding phases subcommands
func (e *Runner) BindToCommand(cmd *cobra.Command) {

This comment has been minimized.

@timothysc

timothysc Oct 12, 2018

Member

Please add a comment that order matters here and that this needs to be done at the end once your phases are all added.

@timothysc

timothysc Oct 12, 2018

Member

Please add a comment that order matters here and that this needs to be done at the end once your phases are all added.

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 12, 2018

Contributor

done

@fabriziopandini

fabriziopandini Oct 12, 2018

Contributor

done

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Oct 12, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, timothysc

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Contributor

k8s-ci-robot commented Oct 12, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, timothysc

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fabriziopandini

This comment has been minimized.

Show comment
Hide comment
@fabriziopandini

fabriziopandini Oct 12, 2018

Contributor

/test pull-kubernetes-e2e-gce

Contributor

fabriziopandini commented Oct 12, 2018

/test pull-kubernetes-e2e-gce

@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123
Member

neolit123 commented Oct 12, 2018

LGTM, thanks @fabriziopandini

@timothysc

/lgtm

@timothysc

This comment has been minimized.

Show comment
Hide comment
@timothysc

timothysc Oct 12, 2018

Member

@fabriziopandini - I look forward to the refactor!

Member

timothysc commented Oct 12, 2018

@fabriziopandini - I look forward to the refactor!

}
// RunData defines the data shared among all the phases included in the workflow, that is any type.
type RunData = interface{}

This comment has been minimized.

@chuckha

chuckha Oct 12, 2018

Member

What is the benefit of the type alias here?

versus type definition like this

type RunData interface{}
@chuckha

chuckha Oct 12, 2018

Member

What is the benefit of the type alias here?

versus type definition like this

type RunData interface{}

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 13, 2018

Contributor

@chuckha
I simply found ugly using interface{}, and thus created an alias for it.

@fabriziopandini

fabriziopandini Oct 13, 2018

Contributor

@chuckha
I simply found ugly using interface{}, and thus created an alias for it.

@k8s-ci-robot k8s-ci-robot merged commit 7d45044 into kubernetes:master Oct 12, 2018

18 checks passed

cla/linuxfoundation fabriziopandini authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@fabriziopandini fabriziopandini deleted the fabriziopandini:kubeadm-add-phase-runner2 branch Oct 13, 2018

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