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

Jump to bottom

add new command kubectl set volume #54284

Closed

WanLinghao wants to merge 1 commit into kubernetes:master from WanLinghao:set_volume

Contributor

WanLinghao commented Oct 20, 2017 •

edited

Loading

What this PR does / why we need it:
#21648
Moved from OpenShift to Kubenetes.
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

Add 'kubectl set volume' command for setting volume in containers for resources embedding pod templates

Contributor

k8s-ci-robot commented Oct 20, 2017

@WanLinghao: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added size/XXL do-not-merge/release-note-label-needed cncf-cla: yes needs-ok-to-test release-note and removed do-not-merge/release-note-label-needed labels

Contributor

CaoShuFeng commented Oct 20, 2017

/ok-to-test

k8s-ci-robot removed the needs-ok-to-test label

k8s-github-robot assigned ericchiang

k8s-github-robot commented Oct 20, 2017

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: WanLinghao
We suggest the following additional approver: pwittrock

Assign the PR to them by writing /assign @pwittrock in a comment when ready.

Associated issue: 21648

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot assigned cblecker

k8s-github-robot added the kind/old-docs label

Member

mkumatag commented Oct 20, 2017

api.Codecs moved to legacyscheme.Codecs part of #53984, so you may need to fix the import path to build the code.

Member

cblecker commented Oct 23, 2017

/unassign
Please reassign back if you need hack/ approval once this has been reviewed by a sig-cli reviewer :)

k8s-ci-robot unassigned cblecker

WanLinghao force-pushed the set_volume branch from 1fd56b5 to e8d2365 Compare

October 27, 2017 02:45

ericchiang mentioned this pull request

Proposal: First-generation of kubectl set * commands #21648

Closed

Contributor

0xmichalis commented Oct 27, 2017

@kubernetes/sig-cli-pr-reviews

k8s-ci-robot added the sig/cli label

Contributor Author

WanLinghao commented Oct 30, 2017

/test pull-kubernetes-unit

Contributor Author

WanLinghao commented Oct 30, 2017

/assign cblecker

k8s-ci-robot assigned cblecker

dixudx reviewed

View reviewed changes

pkg/kubectl/cmd/set/set_volume.go Outdated

    
              const (

              	volumePrefix    = "volume-"

              	storageAnnClass = "volume.beta.kubernetes.io/storage-class"

Member

dixudx Oct 30, 2017

No need to import new storageAnnClass, use api.BetaStorageClassAnnotation instead.

Contributor Author

WanLinghao Oct 30, 2017

OK

pkg/kubectl/cmd/set/set_volume.go Outdated

    
              func (o *VolumeOptions) addVolumeToSpec(spec *api.PodSpec, info *resource.Info, singleResource bool) error {

              	opts := o.AddOpts

              	if len(o.Name) == 0 {

              		var err error

Member

dixudx Oct 30, 2017

Better change to

if o.Name, err := o.getVolumeName(spec, singleResource); err != nil {
	return err
}

Contributor Author

WanLinghao Oct 30, 2017

I think cause o.Name is an existed variable, so your way is not suitable

Contributor Author

WanLinghao Oct 30, 2017

var err error
if o.Name, err = o.getVolumeName(spec, singleResource); err != nil {
	return err
}

did you mean this?

Member

dixudx Oct 30, 2017

SGTM

pkg/kubectl/cmd/set/set_volume.go

    
              			return errors.New("either specify --type or --source but not both for --add operation")

              		}

              		if len(a.Type) > 0 {

Member

dixudx Oct 30, 2017

I wonder shall we split these checking to separate methods to make it more concise and maintainable.

Contributor Author

WanLinghao Oct 30, 2017

pkg/kubectl/cmd/set/set_volume.go

    
              func (a *AddVolumeOptions) Validate(isAddOp bool) error {

              	if isAddOp {

              		if len(a.Type) == 0 && (len(a.ClaimName) > 0 || len(a.ClaimSize) > 0) {

              			a.Type = "persistentvolumeclaim"

Member

dixudx Oct 30, 2017

Actually I don't like such a hard-coded way.

Contributor Author

WanLinghao Oct 30, 2017

@dixudx please make it more clear.Do you mean this function contains too much things?

shiywang suggested changes

View reviewed changes

pkg/kubectl/cmd/set/set_volume.go Outdated

    
              	"k8s.io/apimachinery/pkg/runtime"

              	"k8s.io/apimachinery/pkg/types"

              	"k8s.io/apiserver/pkg/storage/names"

              	"k8s.io/kubernetes/pkg/api"

Contributor

shiywang Oct 30, 2017

no internal type

Contributor Author

WanLinghao Nov 2, 2017

@shiywang I am not clear with version stuff, would please give me some hints, thanks a lot

shiywang reviewed

View reviewed changes

pkg/kubectl/cmd/set/set_volume.go Outdated

    
              	apierrs "k8s.io/apimachinery/pkg/api/errors"

              	"k8s.io/apimachinery/pkg/api/meta"

              	kresource "k8s.io/apimachinery/pkg/api/resource"

Contributor

shiywang Oct 30, 2017

nit: i would prefer just "k8s.io/apimachinery/pkg/api/resource" since there's no upstream dependency anymore.

Contributor Author

WanLinghao Nov 2, 2017

since this file import another resource package "k8s.io/kubernetes/pkg/kubectl/resource"

shiywang reviewed

View reviewed changes

pkg/kubectl/cmd/set/set_volume.go Outdated

    
              }

              func (o *VolumeOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, out, errOut io.Writer) error {

              	kc, err := f.ClientSet()

Contributor

shiywang Oct 30, 2017 •

edited

Loading

nit: in kubernetes, we can just call it client

shiywang reviewed

View reviewed changes

pkg/kubectl/cmd/set/set_volume.go Outdated

    
              		if len(o.AddOpts.ClaimName) == 0 {

              			o.AddOpts.ClaimName = names.SimpleNameGenerator.GenerateName("pvc-")

              		}

              		q, err := kresource.ParseQuantity(o.AddOpts.ClaimSize)

Contributor

shiywang Oct 30, 2017 •

edited

Loading

ditto, kresource

Contributor

zjj2wry Nov 14, 2017

+1

Contributor

shiywang commented Oct 30, 2017 •

edited

Loading

just had a glance, will take a deeper look tomorrow morning

zjj2wry reviewed

View reviewed changes

pkg/kubectl/cmd/set/set_volume_test.go Outdated

    
              	"k8s.io/apimachinery/pkg/api/meta"

              	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

              	"k8s.io/apimachinery/pkg/runtime/schema"

              	"k8s.io/kubernetes/pkg/api"

Contributor

zjj2wry Oct 31, 2017

this should be removed, use versioned api

Contributor Author

WanLinghao Nov 2, 2017

use "k8s.io/client-go/kubernetes/typed/core/v1" replacing "k8s.io/kubernetes/pkg/api"?

Contributor Author

WanLinghao Nov 2, 2017

@zjj2wry since I am still not clear with this version stuff

Contributor

zjj2wry Nov 2, 2017

api rep: k8s.io/api...
api maybe in: k8s.io/api/core/v1

Contributor Author

WanLinghao Nov 2, 2017

@zjj2wry Is it a new move still on pushing? I notice that file in same package like set_env.go, set_image.go still use "k8s.io/kubernetes/pkg/api".
since connections between them exist, if use core/v1 in set_volume.go. then it is necessary that we should use the same in this whole package

Contributor

zjj2wry Nov 2, 2017

yes, will fix all set command :), and it is in process

Contributor Author

WanLinghao Nov 2, 2017

@zjj2wry so would mind if I just ignore this right now and fix it after other set command?
by the way , are there any PR that focus on this? please tell me so I can keep in track

Member

liggitt Nov 2, 2017

it's being actively worked on in #54554

@WanLinghao I would prefer not adding this until that PR is in

Contributor Author

WanLinghao Nov 3, 2017

@liggitt OK, I will wait for this and rebase then

Contributor

zjj2wry commented Nov 14, 2017

@WanLinghao this PR lgtm when your fix small nits.

Contributor

k8s-ci-robot commented Nov 15, 2017 •

edited

Loading

@WanLinghao: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-gpu	1fd56b503cf4498dbbfdd9bea98609c4e063f443	link	`/test pull-kubernetes-e2e-gce-gpu`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.


          this patch add "kubectl set volume" for easier updating container vol…

87d22e0

…umes

	modified:   docs/.generated_docs
	new file:   docs/man/man1/kubectl-set-volumes.1
	new file:   docs/user-guide/kubectl/kubectl_set_volumes.md
	modified:   hack/make-rules/test-cmd-util.sh
	modified:   pkg/kubectl/cmd/set/set.go
	new file:   pkg/kubectl/cmd/set/set_volume_test.go
	new file:   pkg/kubectl/cmd/set/set_volume.go
	modified:   pkg/kubectl/cmd/set/BUILD

WanLinghao force-pushed the set_volume branch from 19327f2 to 87d22e0 Compare

November 15, 2017 03:41

Contributor Author

WanLinghao commented Nov 15, 2017

@zjj2wry @Kargakis ready

Contributor Author

WanLinghao commented Nov 16, 2017

Contributor Author

WanLinghao commented Nov 21, 2017

@zjj2wry do you know who can help push this, thanks

Contributor Author

WanLinghao commented Nov 21, 2017

@liggitt PTAL thanks

Contributor

zjj2wry commented Nov 21, 2017

/retest
@kubernetes/sig-cli-pr-reviews

Member

mengqiy commented Nov 22, 2017

Haven't look at the code closely, but I'm happy this PR doesn't introduce bad dependency.

liggitt added this to the v1.10 milestone

Contributor Author

WanLinghao commented Nov 24, 2017

@liggitt are you ok with this patch? if so , when it can be merged.

Member

dixudx commented Nov 24, 2017

@WanLinghao Code freeze now for v1.9 release. And this is not a must-fix issue for v1.9.

Since it has already been labeled for v1.10, please be patient to wait for the end of release v1.9.

Thanks for your contribution.

Contributor Author

WanLinghao commented Nov 24, 2017

@dixudx OK,thanks

Member

cblecker commented Dec 3, 2017

/unassign

k8s-ci-robot unassigned cblecker

Contributor Author

WanLinghao commented Dec 19, 2017

@smarterclayton @liggitt PTAL

k8s-github-robot added the milestone/incomplete-labels label

Member

thockin commented Mar 5, 2018

This change is

k8s-github-robot added milestone/removed and removed milestone/incomplete-labels labels

k8s-github-robot commented Mar 6, 2018

[MILESTONENOTIFIER] Milestone Removed From Pull Request

@WanLinghao @ericchiang

Important: This pull request was missing labels required for the v1.10 milestone for more than 3 days:

kind: Must specify exactly one of kind/bug, kind/cleanup or kind/feature.
priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.

Help

k8s-github-robot removed this from the v1.10 milestone

fejta-bot commented Jun 4, 2018

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

k8s-ci-robot added the lifecycle/stale label

fejta-bot commented Jul 4, 2018

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

k8s-ci-robot added lifecycle/rotten and removed lifecycle/stale labels

fejta-bot commented Aug 3, 2018

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot closed this

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

Reviewers

zjj2wry zjj2wry left review comments

shiywang shiywang requested changes

dixudx dixudx left review comments

0xmichalis 0xmichalis left review comments

liggitt liggitt left review comments

Labels

cncf-cla: yes lifecycle/rotten milestone/removed release-note sig/cli size/XXL