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

Adding E2E test template #21

Merged
merged 2 commits into from
Aug 17, 2018
Merged

Adding E2E test template #21

merged 2 commits into from
Aug 17, 2018

Conversation

krunaljain
Copy link
Contributor

@krunaljain krunaljain commented Aug 17, 2018 •
edited
Loading

Adding E2E testing template for filestore CSI driver

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 17, 2018
@krunaljain
Copy link
Contributor Author

/assign @msau42

msau42
msau42 reviewed Aug 17, 2018
View reviewed changes
Makefile Outdated
@@ -19,6 +19,8 @@ ifeq ($(VERSION),)
endif

all: image
mkdir -p bin
go build -ldflags "-X main.vendorVersion=${STAGINGVERSION}" -o bin/gcfs-csi-driver ./cmd/
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be VERSION

Makefile Outdated
@@ -19,6 +19,8 @@ ifeq ($(VERSION),)
endif

all: image
mkdir -p bin
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put these under a "make local" rule instead

test/e2e/tests/single_zone_e2e_test.go Outdated

var _ = Describe("GCFS CSI Driver", func() {

It("Should create->attach->stage->mount volume and check if it is writable, then unmount->unstage->detach->delete and check disk is deleted", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove "attach", "stage", "unstage" and "detach" since filestore doesn't implement those operations

test/e2e/tests/single_zone_e2e_test.go Outdated
defaultSizeGb int64 = 5
)

var _ = Describe("GCFS CSI Driver", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Call this "Google Cloud Filestore CSI Driver"

test/e2e/tests/single_zone_e2e_test.go Outdated

err = client.NodePublishVolume(vol.GetId(), publishDir, vol.GetAttributes())
Expect(err).To(BeNil(), "NodePublishVolume failed with error")
err = testutils.ForceChmod(instance, filepath.Join("/tmp/", volName), "777")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be done with the mkdir command above

test/e2e/tests/single_zone_e2e_test.go
volName := testNamePrefix + string(uuid.NewUUID())
vol, err := client.CreateVolume(volName)
Expect(err).To(BeNil(), "CreateVolume failed with error: %v", err)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a TODO here to validate the Filestore instance creation at the cloud provider layer?

@msau42
Copy link
Contributor

msau42 commented Aug 17, 2018

You are missing the Gopkg updates for the vendor changes

@krunaljain krunaljain force-pushed the feature/E2E_tests branch 2 times, most recently from d6a0fee to 20b9616 Compare August 17, 2018 02:35
@msau42
Copy link
Contributor

msau42 commented Aug 17, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 17, 2018
msau42
msau42 reviewed Aug 17, 2018
View reviewed changes
Copy link
Contributor

@msau42 msau42 left a comment

Choose a reason for hiding this comment

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

For some reason the commits from an unrelated merged PR are showing up. You might need to rebase

Makefile Outdated
@@ -20,9 +20,13 @@ endif

all: image

image:
image: make-local
Copy link
Contributor

Choose a reason for hiding this comment

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

i would not make a local build a dependency when doing a container build. please keep docker build and local build separate

Makefile Outdated
docker build --build-arg TAG=$(VERSION) -t $(IMAGE):$(VERSION) .

make-local:
Copy link
Contributor

Choose a reason for hiding this comment

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

i would just call this "local"

Copy link
Contributor

Choose a reason for hiding this comment

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

in the e2e framework, you will have to change the "make" to a "make local"

test/e2e/tests/single_zone_e2e_test.go Outdated
Expect(err).To(BeNil(), "Failed to delete remote directory")
}()

err = testutils.ForceChmod(instance, filepath.Join("/tmp/", volName), "777")
Copy link
Contributor

Choose a reason for hiding this comment

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

Use publishDir.

test/e2e/tests/single_zone_e2e_test.go Outdated

err = client.NodePublishVolume(vol.GetId(), secondPublishDir, vol.GetAttributes())
Expect(err).To(BeNil(), "NodePublishVolume failed with error")
err = testutils.ForceChmod(instance, filepath.Join("/tmp/", volName), "777")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be secondPublishDir and needs to happen before the NodePublishVolume.

Gopkg.lock
@@ -2,46 +2,81 @@


[[projects]]
digest = "1:180876db3ec295bb9f0babec5ca926fe9f2036b747b7c5bfcd13b333023e7cfd"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you also need to include the Gopkg.toml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't get any changes in that file after running dep -- ensure

@msau42
Copy link
Contributor

msau42 commented Aug 17, 2018

lgtm, can you squash and rebase

@krunaljain krunaljain force-pushed the feature/E2E_tests branch 2 times, most recently from 513fbd9 to dc3b17d Compare August 17, 2018 19:17
@msau42
Copy link
Contributor

msau42 commented Aug 17, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krunaljain, msau42

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2018
@k8s-ci-robot k8s-ci-robot merged commit 88d0de8 into kubernetes-sigs:master Aug 17, 2018
@msau42 msau42 mentioned this pull request Aug 18, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msau42 msau42 msau42 left review comments

@saad-ali saad-ali Awaiting requested review from saad-ali

@verult verult Awaiting requested review from verult

Assignees

@msau42 msau42

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@krunaljain @msau42 @k8s-ci-robot