-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
Introduce SaveableListener#onDeleted
#9743
base: master
Are you sure you want to change the base?
Conversation
Usually `Saveable` objects are written, but it can happen on occasion that they get deleted, and it wasn't generating an event for every case. This provides a more fine-grained event that can be handled by implemented listeners. In my case, I have a use case in CloudBees CI where I need to clear a cache entry when a Saveable gets deleted from disk.
Logger.getLogger(SaveableListener.class.getName()).log(Level.WARNING, null, t); | ||
} | ||
} | ||
Listeners.notify(SaveableListener.class, false, l -> l.onChange(o, 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.
Note that the false
is crucial here for things like auditing.
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.
if its crucial should it be tested?
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.
Addressed in 4cf76b9
@@ -556,7 +556,7 @@ public void delete() throws IOException { | |||
loggers.forEach(Target::disable); | |||
|
|||
getParent().getRecorders().forEach(logRecorder -> logRecorder.getLoggers().forEach(Target::enable)); | |||
SaveableListener.fireOnChange(this, getConfigFile()); | |||
SaveableListener.fireOnDeleted(this, getConfigFile()); |
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.
Incompatible, I guess, but unlikely to be a problem?
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.
Yes, I guess it is unlikely enough.
@@ -815,6 +815,7 @@ public void delete() throws IOException, InterruptedException { | |||
ItemDeletion.deregister(this); | |||
} | |||
} | |||
SaveableListener.fireOnDeleted(this, getConfigFile()); |
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.
A related oddity FTR: #9304 (comment)
Logger.getLogger(SaveableListener.class.getName()).log(Level.WARNING, null, t); | ||
} | ||
} | ||
Listeners.notify(SaveableListener.class, false, l -> l.onChange(o, 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.
if its crucial should it be tested?
…tained from the Saveable listener.
/label ready-for-merge This PR is now ready for merge, after the pending security release, we will merge it if there's no negative feedback. Thanks! |
Usually
Saveable
objects are written, but it can happen on occasion that they get deleted, and it wasn't generating an event for every case.This provides a more fine-grained event that can be handled by implemented listeners.
In my case, I have a use case in CloudBees CI where I need to clear a cache entry when a
Saveable
gets deleted from disk.See JENKINS-XXXXX.
Testing done
Proposed changelog entries
SaveableListener#onDeleted
can be implemented by plugins to be notified aboutSaveable
deletion.Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist