-
Notifications
You must be signed in to change notification settings - Fork 40
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
Modified the audit log file name and Added deletion of empty file #503
Conversation
517aecc
to
4b1b16b
Compare
pkg/audit/audit.go
Outdated
@@ -70,3 +71,10 @@ func (a *Audit) Log(name, op, value string) { | |||
} | |||
a.mutex.Unlock() | |||
} | |||
|
|||
func Delete(file string) { | |||
check_file, _ := os.Stat(file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens when file doesn't exist for some reason? its better to check for err and do nothing if error occurred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/audit/audit.go
Outdated
|
||
func Delete(file string) { | ||
check_file, _ := os.Stat(file) | ||
if check_file.Size() == 0 && file == "pvsadm_audit.log" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you wanna check the filename here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to delete the file only if its created by the tool and not to delete the user provided audit file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO its okay to delete the use provided as well if file is empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. will remove the condition and delete the user provided file as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
4b1b16b
to
fdc739e
Compare
cmd/root.go
Outdated
if err := rootCmd.Execute(); err != nil { | ||
klog.Errorln(err) | ||
audit.Delete(pkg.Options.AuditFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we calling this again if its already mentioned above as defer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Defer will not be executed after os.Exit. we are calling the Delete explicitly before that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm..., lets handle the exit in the main() by returning err from here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
fdc739e
to
659a068
Compare
cmd/root.go
Outdated
if err := rootCmd.Execute(); err != nil { | ||
klog.Errorln(err) | ||
audit.Delete(pkg.Options.AuditFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm..., lets handle the exit in the main() by returning err from here
659a068
to
797c524
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KeerthanaAP, mkumatag 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 |
Fix #152