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

Jump to bottom

move csi-common to ceph-csi #207

Merged
merged 2 commits into from
Feb 27, 2019
Merged

move csi-common to ceph-csi #207

merged 2 commits into from
Feb 27, 2019

Conversation

Madhu-1
Copy link
Collaborator

@Madhu-1 Madhu-1 commented Feb 18, 2019

kubernetes/drivers/csi-common is no longer maintained. moving common utility functions to ceph-csi

Signed-off-by: Madhu Rajanna mrajanna@redhat.com

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Feb 18, 2019

@rootfs @gman0 PTAL

gman0
gman0 reviewed Feb 19, 2019
View reviewed changes
pkg/csi-common/controllerserver-default.go Outdated
func (cs *DefaultControllerServer) DeleteSnapshot(ctx context.Context, req *csi.DeleteSnapshotRequest) (*csi.DeleteSnapshotResponse, error) {
return nil, status.Error(codes.Unimplemented, "")
}

// ListSnapshots list snapshosts
Copy link
Contributor

Choose a reason for hiding this comment

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

listS snapshots

@gman0
Copy link
Contributor

gman0 commented Feb 19, 2019

No glog means this needs an update as well:

ceph-csi/pkg/util/log.go

Lines 26 to 45 in 8e41a8a

// InitLogging initializes klog alongside glog
// XXX: This is just a temporary solution till all deps move to klog
func InitLogging() {
if err := flag.Set("logtostderr", "true"); err != nil {
klog.Errorf("failed to set logtostderr flag: %v", err)
os.Exit(1)
}
flag.Parse()
klogFlags := flag.NewFlagSet("klog", flag.ExitOnError)
klog.InitFlags(klogFlags)
// Sync klog flags with glog
flag.CommandLine.VisitAll(func(f1 *flag.Flag) {
if f2 := klogFlags.Lookup(f1.Name); f2 != nil {
f2.Value.Set(f1.Value.String()) // nolint: errcheck, gosec
}
})
}

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Feb 19, 2019

@gman0 can this be done in followup PR?

No glog means this needs an update as well:
ceph-csi/pkg/util/log.go

Lines 26 to 45 in 8e41a8a

// InitLogging initializes klog alongside glog
// XXX: This is just a temporary solution till all deps move to klog
func InitLogging() {
if err := flag.Set("logtostderr", "true"); err != nil {
klog.Errorf("failed to set logtostderr flag: %v", err)
os.Exit(1)
}

flag.Parse()

klogFlags := flag.NewFlagSet("klog", flag.ExitOnError)
klog.InitFlags(klogFlags)

// Sync klog flags with glog
flag.CommandLine.VisitAll(func(f1 *flag.Flag) {
if f2 := klogFlags.Lookup(f1.Name); f2 != nil {
f2.Value.Set(f1.Value.String()) // nolint: errcheck, gosec
}
})
}

@gman0 can this be done in followup PR?

@Madhu-1 Madhu-1 force-pushed the remove-dep branch 2 times, most recently from 6d2fb47 to ccad30e Compare February 19, 2019 14:57
@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Feb 19, 2019

@gman0 removed glog PTAL

@gman0
Copy link
Contributor

gman0 commented Feb 19, 2019

Could you please also remove https://github.com/ceph/ceph-csi/blob/csi-v1.0/pkg/util/log.go ? Not needed anymore.

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Feb 19, 2019

Could you please also remove https://github.com/ceph/ceph-csi/blob/csi-v1.0/pkg/util/log.go ? Not needed anymore.

@gman0 is that file still present? i think it got deleted

@gman0
Copy link
Contributor

gman0 commented Feb 19, 2019

LGTM

humblec
humblec reviewed Feb 20, 2019
View reviewed changes
cmd/cephfs/main.go
@@ -32,8 +33,15 @@ var (
metadataStorage = flag.String("metadatastorage", "", "metadata persistence method [node|k8s_configmap]")
)

func init() {
klog.InitFlags(nil)
if err := flag.Set("logtostderr", "true"); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Madhu-1 did you validate logtostderr works ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@humblec yes tested, am able to see all logs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

cc @gman0

@humblec
Copy link
Collaborator

humblec commented Feb 20, 2019

@Madhu-1 one quick comment

@humblec
Copy link
Collaborator

humblec commented Feb 21, 2019

lgtm.

@Madhu-1
move csi-common to ceph-csi
fdc0d82 
kubernetes/driver/csi-common is no
longer maintained.

Signed-off-by: Madhu Rajanna <mrajanna@redhat.com>
@Madhu-1
remove glog
6f4f148 
Signed-off-by: Madhu Rajanna <mrajanna@redhat.com>
@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Feb 27, 2019

resolved merge conflicts

@rootfs PTAL.

rootfs
rootfs reviewed Feb 27, 2019
View reviewed changes
pkg/cephfs/controllerserver.go
@@ -17,16 +17,15 @@ limitations under the License.
package cephfs

import (
csicommon "github.com/ceph/ceph-csi/pkg/csi-common"
Copy link
Member

Choose a reason for hiding this comment

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

where is this pkg?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

.../pkg/csi-common/controllerserver-default.go → pkg/csi-common/controllerserver-default.go

this package is moved under pkg/csi-commom

Copy link
Member

Choose a reason for hiding this comment

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

I see, github is funny

@rootfs rootfs merged commit 49f5ce2 into ceph:csi-v1.0 Feb 27, 2019
wilmardo pushed a commit to wilmardo/ceph-csi that referenced this pull request Jul 29, 2019
@rootfs
Merge pull request ceph#207 from Madhu-1/remove-dep
8770f62 
move csi-common to ceph-csi
yati1998 pushed a commit to yati1998/ceph-csi that referenced this pull request Feb 20, 2024
@openshift-ci
Merge pull request ceph#207 from red-hat-storage/sync_ds--devel
eafc5ab 
Syncing latest changes from devel for ceph-csi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gman0 gman0 gman0 left review comments

@humblec humblec humblec left review comments

@rootfs rootfs rootfs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Madhu-1 @gman0 @humblec @rootfs