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

check vsc null pointer #5217

Merged
merged 1 commit into from
Aug 27, 2022
Merged

Conversation

lilongfeng0902
Copy link

@lilongfeng0902 lilongfeng0902 commented Aug 16, 2022

Thank you for contributing to Velero!

Please add a summary of your change

Fix a little problem, add a null pointer to judge.

Does your change fix a particular issue?

none
Fixes #(issue)
none

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

Signed-off-by: 李龙峰 [email protected]

@sseago
Copy link
Collaborator

sseago commented Aug 16, 2022

The changelog filename doesn't match the PR, since a new PR was created. You need to rename "changelogs/unreleased/5213-lilongfeng0902" to "changelogs/unreleased/5217-lilongfeng0902" in this PR.

@reasonerjt reasonerjt requested a review from blackpiglet August 16, 2022 15:26
@lilongfeng0902
Copy link
Author

lilongfeng0902 commented Aug 17, 2022

The changelog filename doesn't match the PR, since a new PR was created. You need to rename "changelogs/unreleased/5213-lilongfeng0902" to "changelogs/unreleased/5217-lilongfeng0902" in this PR.

Thanks for your review, I have did it.
Signed-off-by: 李龙峰 [email protected]

@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2022

Codecov Report

Merging #5217 (66b8ed8) into main (5b6d361) will increase coverage by 0.07%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #5217      +/-   ##
==========================================
+ Coverage   41.60%   41.67%   +0.07%     
==========================================
  Files         215      216       +1     
  Lines       18643    18794     +151     
==========================================
+ Hits         7756     7833      +77     
- Misses      10309    10377      +68     
- Partials      578      584       +6     
Impacted Files Coverage Δ
pkg/controller/backup_controller.go 48.26% <0.00%> (-0.22%) ⬇️
pkg/restore/restore.go 64.06% <0.00%> (-0.57%) ⬇️
pkg/install/resources.go 52.31% <0.00%> (-0.25%) ⬇️
pkg/cmd/server/server.go 6.43% <0.00%> (-0.05%) ⬇️
pkg/uploader/types.go 100.00% <0.00%> (ø)
pkg/install/deployment.go 87.27% <0.00%> (+0.35%) ⬆️
pkg/repository/provider/unified_repo.go 54.20% <0.00%> (+1.32%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@blackpiglet
Copy link
Contributor

blackpiglet commented Aug 18, 2022

Thanks for the effort.
Please squash the commit into one, and force push to make the commit list simple and clean.

@lilongfeng0902
Copy link
Author

Thanks for the effort. Please squash the commit into one, and force push to make the commit list simple and clean.

I squash the commit into one, and force push the commit. Thanks.

@@ -951,8 +951,11 @@ func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []*snapshotv1api
if vs.Status.BoundVolumeSnapshotContentName != nil &&
len(*vs.Status.BoundVolumeSnapshotContentName) > 0 {
vsc = vscMap[*vs.Status.BoundVolumeSnapshotContentName]
if vsc.Spec.DeletionPolicy == snapshotv1api.VolumeSnapshotContentDelete {
if nil != vsc && vsc.Spec.DeletionPolicy == snapshotv1api.VolumeSnapshotContentDelete {
Copy link
Contributor

@blackpiglet blackpiglet Aug 18, 2022

Choose a reason for hiding this comment

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

Sorry, I didn't notice this in last review. It should be something like this.

				if nil == vsc {
					logger.Errorf("Not find %s from the vscMap", vs.Status.BoundVolumeSnapshotContentName)
					return
				}
				if vsc.Spec.DeletionPolicy == snapshotv1api.VolumeSnapshotContentDelete {
					modifyVSCFlag = true
				}

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I didn't notice this in last review. It should be something like this.

				if nil == vsc {
					logger.Errorf("Not find %s from the vscMap", vs.Status.BoundVolumeSnapshotContentName)
					return
				}
				if vsc.Spec.DeletionPolicy == snapshotv1api.VolumeSnapshotContentDelete {
					modifyVSCFlag = true
				}

Yes,that's look like good, I have did that. Please check again. Thank you.

blackpiglet
blackpiglet previously approved these changes Aug 18, 2022
@@ -951,6 +951,10 @@ func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []*snapshotv1api
if vs.Status.BoundVolumeSnapshotContentName != nil &&
len(*vs.Status.BoundVolumeSnapshotContentName) > 0 {
vsc = vscMap[*vs.Status.BoundVolumeSnapshotContentName]
if nil != vsc {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be nil== vsc?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you're right. Too careless……

@blackpiglet blackpiglet dismissed their stale review August 22, 2022 06:34

Ignore problematic nil checking.

Signed-off-by: 李龙峰 <[email protected]>
@blackpiglet blackpiglet merged commit 86762f4 into vmware-tanzu:main Aug 27, 2022
@qiuming-best qiuming-best mentioned this pull request Sep 7, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants