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.

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 new command kubectl set volume #54284

Closed
wants to merge 1 commit into from

Conversation

@WanLinghao
Copy link
Contributor

commented Oct 20, 2017

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
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

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.

@CaoShuFeng

This comment has been minimized.

Copy link
Member

commented Oct 20, 2017

/ok-to-test

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

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

@mkumatag

This comment has been minimized.

Copy link
Member

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.

@cblecker

This comment has been minimized.

Copy link
Member

commented Oct 23, 2017

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

@kargakis

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

@WanLinghao

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2017

/test pull-kubernetes-unit

@WanLinghao

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2017

/assign cblecker


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

This comment has been minimized.

Copy link
@dixudx

dixudx Oct 30, 2017

Member

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

This comment has been minimized.

Copy link
@WanLinghao

WanLinghao Oct 30, 2017

Author Contributor

OK

func (o *VolumeOptions) addVolumeToSpec(spec *api.PodSpec, info *resource.Info, singleResource bool) error {
opts := o.AddOpts
if len(o.Name) == 0 {
var err error

This comment has been minimized.

Copy link
@dixudx

dixudx Oct 30, 2017

Member

Better change to

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

This comment has been minimized.

Copy link
@WanLinghao

WanLinghao Oct 30, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@WanLinghao

WanLinghao Oct 30, 2017

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

did you mean this?

This comment has been minimized.

Copy link
@dixudx
return errors.New("either specify --type or --source but not both for --add operation")
}

if len(a.Type) > 0 {

This comment has been minimized.

Copy link
@dixudx

dixudx Oct 30, 2017

Member

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

This comment has been minimized.

Copy link
@WanLinghao

WanLinghao Oct 30, 2017

Author Contributor

@dixudx OK

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"

This comment has been minimized.

Copy link
@dixudx

dixudx Oct 30, 2017

Member

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

This comment has been minimized.

Copy link
@WanLinghao

WanLinghao Oct 30, 2017

Author Contributor

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

"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apiserver/pkg/storage/names"
"k8s.io/kubernetes/pkg/api"

This comment has been minimized.

Copy link
@shiywang

shiywang Oct 30, 2017

Member

no internal type

This comment has been minimized.

Copy link
@WanLinghao

WanLinghao Nov 2, 2017

Author Contributor

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


apierrs "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
kresource "k8s.io/apimachinery/pkg/api/resource"

This comment has been minimized.

Copy link
@shiywang

shiywang Oct 30, 2017

Member

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

This comment has been minimized.

Copy link
@WanLinghao

WanLinghao Nov 2, 2017

Author Contributor

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

}

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

This comment has been minimized.

Copy link
@shiywang

shiywang Oct 30, 2017

Member

nit: in kubernetes, we can just call it client

if len(o.AddOpts.ClaimName) == 0 {
o.AddOpts.ClaimName = names.SimpleNameGenerator.GenerateName("pvc-")
}
q, err := kresource.ParseQuantity(o.AddOpts.ClaimSize)

This comment has been minimized.

Copy link
@shiywang

shiywang Oct 30, 2017

Member

ditto, kresource

This comment has been minimized.

Copy link
@zjj2wry
@shiywang

This comment has been minimized.

Copy link
Member

commented Oct 30, 2017

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

"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"

This comment has been minimized.

Copy link
@zjj2wry

zjj2wry Oct 31, 2017

Member

this should be removed, use versioned api

This comment has been minimized.

Copy link
@WanLinghao

WanLinghao Nov 2, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@WanLinghao

WanLinghao Nov 2, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@zjj2wry

zjj2wry Nov 2, 2017

Member

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

This comment has been minimized.

Copy link
@WanLinghao

WanLinghao Nov 2, 2017

Author Contributor

@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

This comment has been minimized.

Copy link
@zjj2wry

zjj2wry Nov 2, 2017

Member

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

This comment has been minimized.

Copy link
@WanLinghao

WanLinghao Nov 2, 2017

Author Contributor

@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

This comment has been minimized.

Copy link
@liggitt

liggitt Nov 2, 2017

Member

it's being actively worked on in #54554

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

This comment has been minimized.

Copy link
@WanLinghao

WanLinghao Nov 3, 2017

Author Contributor

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

@zjj2wry

This comment has been minimized.

Copy link
Member

commented Nov 14, 2017

@WanLinghao this PR lgtm when your fix small nits.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

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

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-gpu 1fd56b5 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…
…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 WanLinghao force-pushed the WanLinghao:set_volume branch from 19327f2 to 87d22e0 Nov 15, 2017

@WanLinghao

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2017

@WanLinghao

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2017

@liggitt PTAL

@WanLinghao

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2017

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

@WanLinghao

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2017

@liggitt PTAL thanks

@zjj2wry

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

@mengqiy

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

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

@liggitt liggitt added this to the v1.10 milestone Nov 22, 2017

@WanLinghao

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2017

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

@dixudx

This comment has been minimized.

Copy link
Member

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.

@WanLinghao

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2017

@dixudx OK,thanks

@cblecker

This comment has been minimized.

Copy link
Member

commented Dec 3, 2017

/unassign

@WanLinghao

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

@thockin

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

This change is Reviewable

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

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 k8s-github-robot removed this from the v1.10 milestone Mar 6, 2018

@fejta-bot

This comment has been minimized.

Copy link

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

@fejta-bot

This comment has been minimized.

Copy link

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

@fejta-bot

This comment has been minimized.

Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.