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

Use error type instead of error code in cleanup #497

Merged
merged 1 commit into from
Jan 24, 2024
Merged

Use error type instead of error code in cleanup #497

merged 1 commit into from
Jan 24, 2024

Conversation

seanpang-vmware
Copy link
Contributor

  1. Status impl error interface, so the return value of cleanup changed from status, error to error only.
  2. Provide go doc on Cleanup func to provide more details about returned error type.
  3. Remove OK status, when cleanup success, just return nil error.

@zhengxiexie zhengxiexie mentioned this pull request Jan 23, 2024
Merged
ihgann
ihgann suggested changes Jan 23, 2024
View reviewed changes
Copy link

@ihgann ihgann left a comment

Choose a reason for hiding this comment

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

Overall, LGTM. A few additional suggestions. Can we add unit tests as well?

cmd_clean/main.go Show resolved Hide resolved
pkg/clean/clean.go Outdated Show resolved Hide resolved
pkg/clean/clean.go Show resolved Hide resolved
pkg/clean/clean.go Outdated Show resolved Hide resolved
@seanpang-vmware
Copy link
Contributor Author

Overall, LGTM. A few additional suggestions. Can we add unit tests as well?

Xie will add Unit test for cleanup in another patch

@seanpang-vmware
Use error type instead of error code in cleanup
9accc65 
1. Status impl error interface, so the return value of cleanup changed
from status, error to error only.
2. Provide go doc on Cleanup func to provide more details about returned
error type.
3. Remove OK status, when cleanup success, just return nil error.
zhengxiexie
zhengxiexie approved these changes Jan 24, 2024
View reviewed changes
Copy link
Contributor

@zhengxiexie zhengxiexie left a comment

Choose a reason for hiding this comment

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

LGTM

@seanpang-vmware seanpang-vmware merged commit d77ed9b into vmware-tanzu:main Jan 24, 2024
2 checks passed
@seanpang-vmware seanpang-vmware deleted the errortype branch January 24, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ihgann ihgann ihgann requested changes

@zhengxiexie zhengxiexie zhengxiexie approved these changes

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@seanpang-vmware @zhengxiexie @ihgann @vmwclabot