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 kubectl wait #64034

Merged
merged 3 commits into from
May 23, 2018
+1,177 −24
Merged

add kubectl wait #64034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7679464
add wait
deads2k May 18, 2018
0d0863e
generated
deads2k May 21, 2018
4925859
add a discarding printer for testing and delegation
deads2k May 22, 2018
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
3 changes: 3 additions & 0 deletions build/visible_to/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ package_group(
"//pkg/kubectl/cmd/templates",
"//pkg/kubectl/cmd/util",
"//pkg/kubectl/cmd/util/sanity",
"//pkg/kubectl/cmd/wait",
],
)

Expand All @@ -196,6 +197,7 @@ package_group(
"//pkg/kubectl/cmd/get",
"//pkg/kubectl/cmd/rollout",
"//pkg/kubectl/cmd/set",
"//pkg/kubectl/cmd/wait",
"//pkg/kubectl/explain",
],
)
Expand Down Expand Up @@ -230,6 +232,7 @@ package_group(
"//pkg/kubectl/cmd/testing",
"//pkg/kubectl/cmd/util",
"//pkg/kubectl/cmd/util/editor",
"//pkg/kubectl/cmd/wait",
],
)

Expand Down
3 changes: 3 additions & 0 deletions docs/.generated_docs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ docs/man/man1/kubectl-top-pod.1
docs/man/man1/kubectl-top.1
docs/man/man1/kubectl-uncordon.1
docs/man/man1/kubectl-version.1
docs/man/man1/kubectl-wait.1
docs/man/man1/kubectl.1
docs/man/man1/kubelet.1
docs/user-guide/kubectl/kubectl.md
Expand Down Expand Up @@ -357,6 +358,7 @@ docs/user-guide/kubectl/kubectl_top_node.md
docs/user-guide/kubectl/kubectl_top_pod.md
docs/user-guide/kubectl/kubectl_uncordon.md
docs/user-guide/kubectl/kubectl_version.md
docs/user-guide/kubectl/kubectl_wait.md
docs/yaml/kubectl/kubectl.yaml
docs/yaml/kubectl/kubectl_alpha.yaml
docs/yaml/kubectl/kubectl_annotate.yaml
Expand Down Expand Up @@ -400,3 +402,4 @@ docs/yaml/kubectl/kubectl_taint.yaml
docs/yaml/kubectl/kubectl_top.yaml
docs/yaml/kubectl/kubectl_uncordon.yaml
docs/yaml/kubectl/kubectl_version.yaml
docs/yaml/kubectl/kubectl_wait.yaml
3 changes: 3 additions & 0 deletions docs/man/man1/kubectl-wait.1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
This file is autogenerated, but we've stopped checking such files into the
repository to reduce the need for rebases. Please run hack/generate-docs.sh to
populate this file.
3 changes: 3 additions & 0 deletions docs/user-guide/kubectl/kubectl_wait.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
This file is autogenerated, but we've stopped checking such files into the
repository to reduce the need for rebases. Please run hack/generate-docs.sh to
populate this file.
3 changes: 3 additions & 0 deletions docs/yaml/kubectl/kubectl_wait.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
This file is autogenerated, but we've stopped checking such files into the
repository to reduce the need for rebases. Please run hack/generate-docs.sh to
populate this file.
1 change: 1 addition & 0 deletions hack/.golint_failures
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ pkg/kubectl/cmd/util
pkg/kubectl/cmd/util/editor
pkg/kubectl/cmd/util/jsonmerge
pkg/kubectl/cmd/util/sanity
pkg/kubectl/cmd/wait
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the golang "standard" of everything being New or Interface or Flags sucks. Search for anything like that in our codebase and you're screwed. golint is trying to enforce that on this package, so I'm blacklisting it all.

Also, who thought that was ever a good idea???

pkg/kubectl/genericclioptions
pkg/kubectl/genericclioptions/printers
pkg/kubectl/genericclioptions/resource
Expand Down
6 changes: 5 additions & 1 deletion hack/make-rules/test-cmd-util.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2382,7 +2382,11 @@ run_namespace_tests() {
# Post-condition: namespace 'my-namespace' is created.
kube::test::get_object_assert 'namespaces/my-namespace' "{{$id_field}}" 'my-namespace'
# Clean up
kubectl delete namespace my-namespace
kubectl delete namespace my-namespace --wait=false
# make sure that wait properly waits for finalization
kubectl wait --for=delete ns/my-namespace
Copy link

Choose a reason for hiding this comment

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

There is a chance that the deletion (as a result of kubectl delete above) will succeed before we run this kubectl wait command, i.e. this test could succeed even if the wait command is not implemented correctly.

To make this test case more reliable, we could start kubectl wait --for=delete first, and only then execute kubectl delete in a separate thread.
Not sure how hard it would be to implement with a shell script though, but we can write a similar unit test in Go (in addition to this change).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't super important, the unit tests on this command are quite thorough compared to others

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I chose a namespace because they tend to delete very slowly because of finalizers

output_message=$(! kubectl get ns/my-namespace 2>&1 "${kube_flags[@]}")
kube::test::if_has_string "${output_message}" ' not found'

######################
# Pods in Namespaces #
Expand Down
2 changes: 2 additions & 0 deletions pkg/kubectl/cmd/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ go_library(
"//pkg/kubectl/cmd/util:go_default_library",
"//pkg/kubectl/cmd/util/editor:go_default_library",
"//pkg/kubectl/cmd/util/openapi:go_default_library",
"//pkg/kubectl/cmd/wait:go_default_library",
"//pkg/kubectl/explain:go_default_library",
"//pkg/kubectl/genericclioptions:go_default_library",
"//pkg/kubectl/genericclioptions/printers:go_default_library",
Expand Down Expand Up @@ -264,6 +265,7 @@ filegroup(
"//pkg/kubectl/cmd/testdata/edit:all-srcs",
"//pkg/kubectl/cmd/testing:all-srcs",
"//pkg/kubectl/cmd/util:all-srcs",
"//pkg/kubectl/cmd/wait:all-srcs",
],
tags = ["automanaged"],
visibility = [
Expand Down
6 changes: 5 additions & 1 deletion pkg/kubectl/cmd/apply.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,11 @@ func (o *ApplyOptions) Complete(f cmdutil.Factory, cmd *cobra.Command) error {
return err
}

o.DeleteOptions = o.DeleteFlags.ToOptions(o.IOStreams)
dynamicClient, err := f.DynamicClient()
if err != nil {
return err
}
o.DeleteOptions = o.DeleteFlags.ToOptions(dynamicClient, o.IOStreams)
o.ShouldIncludeUninitialized = cmdutil.ShouldIncludeUninitialized(cmd, o.Prune)

o.OpenAPISchema, _ = f.OpenAPISchema()
Expand Down
10 changes: 10 additions & 0 deletions pkg/kubectl/cmd/apply_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,7 @@ func TestApplyObject(t *testing.T) {
}
tf.OpenAPISchemaFunc = fn
tf.Namespace = "test"
tf.ClientConfigVal = defaultClientConfig()

ioStreams, _, buf, errBuf := genericclioptions.NewTestIOStreams()
cmd := NewCmdApply("kubectl", tf, ioStreams)
Expand Down Expand Up @@ -587,6 +588,7 @@ func TestApplyObjectOutput(t *testing.T) {
}
tf.OpenAPISchemaFunc = fn
tf.Namespace = "test"
tf.ClientConfigVal = defaultClientConfig()

ioStreams, _, buf, errBuf := genericclioptions.NewTestIOStreams()
cmd := NewCmdApply("kubectl", tf, ioStreams)
Expand Down Expand Up @@ -648,6 +650,7 @@ func TestApplyRetry(t *testing.T) {
}
tf.OpenAPISchemaFunc = fn
tf.Namespace = "test"
tf.ClientConfigVal = defaultClientConfig()

ioStreams, _, buf, errBuf := genericclioptions.NewTestIOStreams()
cmd := NewCmdApply("kubectl", tf, ioStreams)
Expand Down Expand Up @@ -697,6 +700,7 @@ func TestApplyNonExistObject(t *testing.T) {
}),
}
tf.Namespace = "test"
tf.ClientConfigVal = defaultClientConfig()

ioStreams, _, buf, _ := genericclioptions.NewTestIOStreams()
cmd := NewCmdApply("kubectl", tf, ioStreams)
Expand Down Expand Up @@ -749,6 +753,7 @@ func TestApplyEmptyPatch(t *testing.T) {
}),
}
tf.Namespace = "test"
tf.ClientConfigVal = defaultClientConfig()

// 1. apply non exist object
ioStreams, _, buf, _ := genericclioptions.NewTestIOStreams()
Expand Down Expand Up @@ -823,6 +828,7 @@ func testApplyMultipleObjects(t *testing.T, asList bool) {
}
tf.OpenAPISchemaFunc = fn
tf.Namespace = "test"
tf.ClientConfigVal = defaultClientConfig()

ioStreams, _, buf, errBuf := genericclioptions.NewTestIOStreams()
cmd := NewCmdApply("kubectl", tf, ioStreams)
Expand Down Expand Up @@ -923,6 +929,7 @@ func TestApplyNULLPreservation(t *testing.T) {
}
tf.OpenAPISchemaFunc = fn
tf.Namespace = "test"
tf.ClientConfigVal = defaultClientConfig()

ioStreams, _, buf, errBuf := genericclioptions.NewTestIOStreams()
cmd := NewCmdApply("kubectl", tf, ioStreams)
Expand Down Expand Up @@ -989,6 +996,7 @@ func TestUnstructuredApply(t *testing.T) {
}
tf.OpenAPISchemaFunc = fn
tf.Namespace = "test"
tf.ClientConfigVal = defaultClientConfig()

ioStreams, _, buf, errBuf := genericclioptions.NewTestIOStreams()
cmd := NewCmdApply("kubectl", tf, ioStreams)
Expand Down Expand Up @@ -1054,6 +1062,7 @@ func TestUnstructuredIdempotentApply(t *testing.T) {
}
tf.OpenAPISchemaFunc = fn
tf.Namespace = "test"
tf.ClientConfigVal = defaultClientConfig()

ioStreams, _, buf, errBuf := genericclioptions.NewTestIOStreams()
cmd := NewCmdApply("kubectl", tf, ioStreams)
Expand Down Expand Up @@ -1223,6 +1232,7 @@ func TestForceApply(t *testing.T) {
tf := cmdtesting.NewTestFactory()
defer tf.Cleanup()

tf.ClientConfigVal = defaultClientConfig()
tf.UnstructuredClient = &fake.RESTClient{
NegotiatedSerializer: unstructuredSerializer,
Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
Expand Down
2 changes: 2 additions & 0 deletions pkg/kubectl/cmd/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"k8s.io/kubernetes/pkg/kubectl/cmd/set"
"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
"k8s.io/kubernetes/pkg/kubectl/cmd/wait"
"k8s.io/kubernetes/pkg/kubectl/util/i18n"

"github.com/spf13/cobra"
Expand Down Expand Up @@ -362,6 +363,7 @@ func NewKubectlCommand(in io.Reader, out, err io.Writer) *cobra.Command {
NewCmdApply("kubectl", f, ioStreams),
NewCmdPatch(f, ioStreams),
NewCmdReplace(f, ioStreams),
wait.NewCmdWait(f, ioStreams),
NewCmdConvert(f, ioStreams),
},
},
Expand Down
53 changes: 49 additions & 4 deletions pkg/kubectl/cmd/delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,20 @@ import (
"strings"
"time"

"github.com/golang/glog"
"github.com/spf13/cobra"

"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/dynamic"
"k8s.io/kubernetes/pkg/kubectl"
"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
kubectlwait "k8s.io/kubernetes/pkg/kubectl/cmd/wait"
"k8s.io/kubernetes/pkg/kubectl/genericclioptions"
"k8s.io/kubernetes/pkg/kubectl/genericclioptions/printers"
"k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource"
"k8s.io/kubernetes/pkg/kubectl/util/i18n"
)
Expand Down Expand Up @@ -106,8 +110,9 @@ type DeleteOptions struct {

Output string

Mapper meta.RESTMapper
Result *resource.Result
DynamicClient dynamic.Interface
Mapper meta.RESTMapper
Result *resource.Result

genericclioptions.IOStreams
}
Expand All @@ -122,7 +127,7 @@ func NewCmdDelete(f cmdutil.Factory, streams genericclioptions.IOStreams) *cobra
Long: delete_long,
Example: delete_example,
Run: func(cmd *cobra.Command, args []string) {
o := deleteFlags.ToOptions(streams)
o := deleteFlags.ToOptions(nil, streams)
Copy link

Choose a reason for hiding this comment

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

Can you just pass f.DynamicClient() there and remove the code setting DeleteOptions.DynamicClient = f.DynamicClient() from DeleteOptions.Complete() method? It took me a while to understand why do you pass nil there.
It would at least make the code more unified across multiple use cases, and we can require the dynamicClient argument to never be nil...

Copy link

Choose a reason for hiding this comment

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

Another option is to pass f.DynamicClient() there, as well as keep the code overwriting it in Complete() (which will set it to the same value). While redundant, it will at least be consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Completing command's options in Complete is a pattern we're enforcing throughout entire kubectl code base.

Copy link

@nilebox nilebox May 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@soltysh yes, but for some reason we don't invoke Complete when we do DeleteFlags.ToOptions in apply and replace, and that's why David had to change the signature of DeleteFlags.ToOptions to make sure that dynamic client is initialized for those commands.

if err := o.Complete(f, args, cmd); err != nil {
cmdutil.CheckErr(err)
}
Expand All @@ -138,6 +143,8 @@ func NewCmdDelete(f cmdutil.Factory, streams genericclioptions.IOStreams) *cobra

deleteFlags.AddFlags(cmd)

cmd.Flags().Bool("wait", true, `If true, wait for resources to be gone before returning. This waits for finalizers.`)
Copy link

Choose a reason for hiding this comment

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

Why not include this flag into deleteFlags.AddFlags(...) method above?

Copy link

Choose a reason for hiding this comment

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

Shall we also say in the description that this flag is on by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall we also say in the description that this flag is on by default?

Help lists defaults already. Delete is a wasteland of weird delegation. I don't think this needs plumbing that far. DeleteFlags is really confused about what it is supposed to be.


cmdutil.AddIncludeUninitializedFlag(cmd)
return cmd
}
Expand Down Expand Up @@ -167,6 +174,9 @@ func (o *DeleteOptions) Complete(f cmdutil.Factory, args []string, cmd *cobra.Co
o.WaitForDeletion = true
Copy link

Choose a reason for hiding this comment

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

As I noted in the other comment, this line becomes obsolete: WaitForDeletion is already true by default. And if user explicitly specifies --wait=false, it will be overwritten a few lines below.

o.GracePeriod = 1
}
if b, err := cmd.Flags().GetBool("wait"); err == nil {
Copy link

Choose a reason for hiding this comment

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

If I understand correctly, there is no real case when err != nil (if there are no bugs in code)? If so, setting o.WaitForDeletion = true 2 lines above becomes useless with this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It happens during bad delegation and this command is ripe with it

o.WaitForDeletion = b
Copy link

Choose a reason for hiding this comment

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

So this seems to be the reason for defining the "wait" flag differently from the rest - reusing the existing WaitForDeletion flag that is currently not exposed to the user?
The only place where the existing internal WaitForDeletion flag is used (for read) is in

kubernetes/pkg/kubectl/cmd/delete.go

Line 259 in 0db40da

if o.WaitForDeletion {
, i.e. in the ReapResult for resources with reapers only. It's not used for resources that don't have reapers.
Given the upcoming change to get rid of reapers completely in #63979, I would suggest not to bother with merging existing and new flags.
Instead, I would just rename the existing flag to get rid of it completely later as part of #63979, as I did in https://github.com/kubernetes/kubernetes/pull/63695/files#diff-7c126b9106a83157d89a336103eb3dbbR108

Copy link

Choose a reason for hiding this comment

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

Actually, once we add an explicit --wait flag and set it to true by default, I don't think we need to keep the existing logic for backward compatibility at all?

I mean the condition

kubernetes/pkg/kubectl/cmd/delete.go

Lines 163 to 168 in 0db40da

if o.GracePeriod == 0 && !o.ForceDeletion {
// To preserve backwards compatibility, but prevent accidental data loss, we convert --grace-period=0
// into --grace-period=1 and wait until the object is successfully deleted. Users may provide --force
// to bypass this wait.
o.WaitForDeletion = true
o.GracePeriod = 1
(and 2 other similar places).
With the changes in this PR, WaitForDeletion is already true by default, i.e. it's consistent with the backward compatibility for grace period. And if the user explicitly sets --wait=false, we shouldn't override it there, I think?

Copy link

Choose a reason for hiding this comment

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

To be clear about the change I propose: Get rid of the existing logic with conditions for setting internal WaitForDeletion to true completely, and declare a new --wait flag that is exposed to the user in the same way as other flags in deleteFlags.AddFlags(...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once reapers die, perhaps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's the goal of getting rid of reapers. The old logic will die.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this seems to be the reason for defining the "wait" flag differently from the rest - reusing the existing WaitForDeletion flag that is currently not exposed to the user?

I think it falls out in the other direction. Reapers die and we remove this old codepath.

Copy link

Choose a reason for hiding this comment

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

Once the reapers are dead, we can move the wait flag to deleteFlags.AddFlags(...) to make it consistent with other flags, since the default will always be true, and there won't be internal conditional "graceful deletion" anymore.

}

o.Reaper = f.Reaper

Expand Down Expand Up @@ -194,6 +204,11 @@ func (o *DeleteOptions) Complete(f cmdutil.Factory, args []string, cmd *cobra.Co
return err
}

o.DynamicClient, err = f.DynamicClient()
Copy link

Choose a reason for hiding this comment

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

I suggest that we remove this code and always require DynamicClient to be non-nil in DeleteFlags.ToOptions() method.
Alternatively, we can make all other commands (apply, replace etc) using DeleteOptions to also invoke this Complete method, and then we can remove the dynamicClient parameter from DeleteFlags.ToOptions() method...

if err != nil {
return err
}

return nil
}

Expand Down Expand Up @@ -300,8 +315,38 @@ func (o *DeleteOptions) DeleteResult(r *resource.Result) error {
}
if found == 0 {
fmt.Fprintf(o.Out, "No resources found\n")
return nil
}
return nil
if !o.WaitForDeletion {
return nil
}
// if we don't have a dynamic client, we don't want to wait. Eventually when delete is cleaned up, this will likely
// drop out.
if o.DynamicClient == nil {
return nil
}

effectiveTimeout := o.Timeout
if effectiveTimeout == 0 {
// if we requested to wait forever, set it to a week.
effectiveTimeout = 168 * time.Hour
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure a week is needed 😉 I'd go with previous 5min being sufficient.

}
waitOptions := kubectlwait.WaitOptions{
ResourceFinder: kubectlwait.ResourceFinderForResult(o.Result),
DynamicClient: o.DynamicClient,
Timeout: effectiveTimeout,

Printer: printers.NewDiscardingPrinter(),
ConditionFn: kubectlwait.IsDeleted,
IOStreams: o.IOStreams,
}
err = waitOptions.RunWait()
if errors.IsForbidden(err) {
// if we're forbidden from waiting, we shouldn't fail.
glog.V(1).Info(err)
return nil
}
return err
}

func (o *DeleteOptions) cascadingDeleteResource(info *resource.Info) error {
Expand Down
6 changes: 4 additions & 2 deletions pkg/kubectl/cmd/delete_flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (

"github.com/spf13/cobra"

"k8s.io/client-go/dynamic"
"k8s.io/kubernetes/pkg/kubectl"
"k8s.io/kubernetes/pkg/kubectl/genericclioptions"
"k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource"
Expand Down Expand Up @@ -72,9 +73,10 @@ type DeleteFlags struct {
Output *string
}

func (f *DeleteFlags) ToOptions(streams genericclioptions.IOStreams) *DeleteOptions {
func (f *DeleteFlags) ToOptions(dynamicClient dynamic.Interface, streams genericclioptions.IOStreams) *DeleteOptions {
Copy link

@nilebox nilebox May 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So passing dynamicClient here is really only necessary for the other commands that use deletion (apply, replace), normal delete command will initialize its DeleteOptions.DynamicClient field in the Complete() method...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but we're doing this so that other places in the code re-use and not create their own versions of wait.

options := &DeleteOptions{
IOStreams: streams,
DynamicClient: dynamicClient,
IOStreams: streams,
}

// add filename options
Expand Down
6 changes: 5 additions & 1 deletion pkg/kubectl/cmd/replace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,11 @@ func (o *ReplaceOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []
return printer.PrintObj(obj, o.Out)
}

deleteOpts := o.DeleteFlags.ToOptions(o.IOStreams)
dynamicClient, err := f.DynamicClient()
if err != nil {
return err
}
deleteOpts := o.DeleteFlags.ToOptions(dynamicClient, o.IOStreams)

//Replace will create a resource if it doesn't exist already, so ignore not found error
deleteOpts.IgnoreNotFound = true
Expand Down
2 changes: 1 addition & 1 deletion pkg/kubectl/cmd/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func (o *RunOptions) Complete(f cmdutil.Factory, cmd *cobra.Command) error {
return printer.PrintObj(obj, o.Out)
}

deleteOpts := o.DeleteFlags.ToOptions(o.IOStreams)
deleteOpts := o.DeleteFlags.ToOptions(o.DynamicClient, o.IOStreams)
deleteOpts.IgnoreNotFound = true
deleteOpts.WaitForDeletion = false
deleteOpts.GracePeriod = -1
Expand Down
4 changes: 2 additions & 2 deletions pkg/kubectl/cmd/run_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ func TestRunArgsFollowDashRules(t *testing.T) {
deleteFlags := NewDeleteFlags("to use to replace the resource.")
opts := &RunOptions{
PrintFlags: printFlags,
DeleteOptions: deleteFlags.ToOptions(genericclioptions.NewTestIOStreamsDiscard()),
DeleteOptions: deleteFlags.ToOptions(nil, genericclioptions.NewTestIOStreamsDiscard()),

IOStreams: genericclioptions.NewTestIOStreamsDiscard(),

Expand Down Expand Up @@ -376,7 +376,7 @@ func TestGenerateService(t *testing.T) {
deleteFlags := NewDeleteFlags("to use to replace the resource.")
opts := &RunOptions{
PrintFlags: printFlags,
DeleteOptions: deleteFlags.ToOptions(genericclioptions.NewTestIOStreamsDiscard()),
DeleteOptions: deleteFlags.ToOptions(nil, genericclioptions.NewTestIOStreamsDiscard()),

IOStreams: ioStreams,

Expand Down
Loading