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

Refactor common controller tests to use withXYZ functions and add tests #254

Merged
merged 2 commits into from
Feb 19, 2020
Merged

Refactor common controller tests to use withXYZ functions and add tests #254

merged 2 commits into from
Feb 19, 2020

Conversation

ggriffiths
Copy link
Member

@ggriffiths ggriffiths commented Feb 12, 2020 •
edited
Loading

Signed-off-by: Grant Griffiths grant@portworx.com

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
refactor finalizer-specific test to use withSnapshotFinalizers function

Which issue(s) this PR fixes:
Fixes #253
Part of #196

Special notes for your reviewer:
n/a

Does this PR introduce a user-facing change?:

NONE

@ggriffiths
Refactor delete test to use withSnapshotFinalizers function
7811e59 
Signed-off-by: Grant Griffiths <grant@portworx.com>
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 12, 2020
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 12, 2020
@ggriffiths ggriffiths removed the request for review from jingxu97 February 12, 2020 23:47
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 13, 2020
@ggriffiths
Copy link
Member Author

Also just added more tests as well. Coverage for snapshot_controller.go is now 74.1%, up from 68.2%.

@ggriffiths ggriffiths force-pushed the common-ctrl-test-refactor branch 3 times, most recently from a04d964 to 20eb761 Compare February 13, 2020 03:06
@ggriffiths ggriffiths changed the title Refactor delete test to use withSnapshotFinalizers function Refactor common controller tests to use withXYZ functions and add tests Feb 13, 2020
xing-yang
xing-yang reviewed Feb 14, 2020
View reviewed changes
pkg/common-controller/framework_test.go
@@ -863,7 +876,7 @@ func newContentWithUnmatchDriverArray(contentName, boundToSnapshotUID, boundToSn
func newSnapshot(
snapshotName, snapshotUID, pvcName, targetContentName, snapshotClassName, boundContentName string,
readyToUse *bool, creationTime *metav1.Time, restoreSize *resource.Quantity,
err *crdv1.VolumeSnapshotError, nilStatus bool, withFinalizer bool, deletionTimestamp *metav1.Time) *crdv1.VolumeSnapshot {
err *crdv1.VolumeSnapshotError, nilStatus bool, withAllFinalizers bool, deletionTimestamp *metav1.Time) *crdv1.VolumeSnapshot {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought the test framework in pv/pvc controller will allow you to create a basic newSnapshot() without passing in the withAllFinalizers flag, and then we can have another function withAllFinalizers() which takes a created snapshot object as input and apply finalizers on top of it. So that means we shouldn't need "withAllFinalizers bool" as an input parameter for newSnapshot().

Copy link
Member Author

Choose a reason for hiding this comment

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

I left "withFinalizers" as "withAllFinalizers" for the purpose of reducing the refactor required. Some tests require all finalizers while others require no finalizers, so pretty much every test would have the "withSnapFinalizers" wrapper.

I mainly introduced "func withSnapshotFinalizers()" to fine-tune which finalizers are added. i.e. for cases where only one finalizer is wanted.

pkg/common-controller/snapshot_delete_test.go Outdated Show resolved Hide resolved
pkg/common-controller/snapshot_update_test.go Show resolved Hide resolved
@ggriffiths ggriffiths force-pushed the common-ctrl-test-refactor branch from 20eb761 to 2409028 Compare February 14, 2020 19:34
xing-yang
xing-yang reviewed Feb 15, 2020
View reviewed changes
pkg/common-controller/framework_test.go Outdated
if withFinalizer {
return withSnapshotFinalizer(&snapshot)
if withAllFinalizers {
return withSnapshotFinalizers([]*crdv1.VolumeSnapshot{&snapshot}, utils.VolumeSnapshotContentFinalizer, utils.VolumeSnapshotAsSourceFinalizer, utils.VolumeSnapshotBoundFinalizer)[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

withSnapshotFinalizers() only adds finalizers on VolumeSnapshot API object, right? Why is utils.VolumeSnapshotContentFinalizer needed here? utils.VolumeSnapshotContentFinalizer is added on VolumeSnapshotContent only.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, removed the contentFinalizer.

xing-yang
xing-yang reviewed Feb 15, 2020
View reviewed changes
pkg/common-controller/framework_test.go
@@ -164,16 +164,17 @@ type reactorError struct {
error error
}

func withSnapshotFinalizer(snapshot *crdv1.VolumeSnapshot) *crdv1.VolumeSnapshot {
snapshot.ObjectMeta.Finalizers = append(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I see that utils.VolumeSnapshotContentFinalizer is added to VolumeSnapshot API object here. This is wrong. We only need utils.VolumeSnapshotContentFinalizer for VolumeSnapshotContent, not for VolumeSnapshot.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, removed the contentFinalizer.

@ggriffiths
Add more snapshot and content sync tests
05efba2 
Signed-off-by: Grant Griffiths <grant@portworx.com>
@ggriffiths ggriffiths force-pushed the common-ctrl-test-refactor branch from 2409028 to 05efba2 Compare February 18, 2020 22:46
@xing-yang
Copy link
Collaborator

/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 Feb 19, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ggriffiths, xing-yang

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 Feb 19, 2020
@k8s-ci-robot k8s-ci-robot merged commit ec9dda7 into kubernetes-csi:master Feb 19, 2020
andyzhangx pushed a commit to andyzhangx/external-snapshotter that referenced this pull request Jun 5, 2024
@k8s-ci-robot
Merge pull request kubernetes-csi#254 from bells17/add-github-actions
4967685 
Add GitHub Actions workflows
andyzhangx added a commit to andyzhangx/external-snapshotter that referenced this pull request Jun 5, 2024
@andyzhangx
Squashed 'release-tools/' changes from adb3af9..f40f0cc
5666da4 
f40f0cc Merge pull request kubernetes-csi#256 from solumath/master
cfa9210 Instruction update
379a1bb Merge pull request kubernetes-csi#255 from humblec/sidecar-md
a5667bb fix typo in sidecar release process
4967685 Merge pull request kubernetes-csi#254 from bells17/add-github-actions
d9bd160 Update skip list in codespell GitHub Action
f5aebfc Add GitHub Actions workflows

git-subtree-dir: release-tools
git-subtree-split: f40f0cc
andyzhangx added a commit to andyzhangx/external-snapshotter that referenced this pull request Aug 14, 2024
@andyzhangx
Squashed 'release-tools/' changes from adb3af9..988496a1f
bb83760 
988496a1f Merge pull request kubernetes-csi#257 from jakobmoellerdev/csi-prow-sidecar-e2e-path
028f8c698 chore: bump to Go 1.22.5
69bd71e8a chore: add CSI_PROW_SIDECAR_E2E_PATH
f40f0cc Merge pull request kubernetes-csi#256 from solumath/master
cfa9210 Instruction update
379a1bb Merge pull request kubernetes-csi#255 from humblec/sidecar-md
a5667bb fix typo in sidecar release process
4967685 Merge pull request kubernetes-csi#254 from bells17/add-github-actions
d9bd160 Update skip list in codespell GitHub Action
f5aebfc Add GitHub Actions workflows

git-subtree-dir: release-tools
git-subtree-split: 988496a1fc3849ed793e03012fdd56813d13d46c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xing-yang xing-yang xing-yang left review comments

Assignees

@xing-yang xing-yang

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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor common-controller tests to use test model similar to in-tree PV tests
3 participants
@ggriffiths @xing-yang @k8s-ci-robot