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

Pull up AbstractFolder.delete logic into AbstractItem #8645

Merged
merged 2 commits into from
Nov 16, 2023

Conversation

jglick
Copy link
Member

@jglick jglick commented Oct 25, 2023

Refactoring of #2789 since jenkinsci/cloudbees-folder-plugin#95 copied a bunch of complex code. As seen in jenkinsci/cloudbees-folder-plugin@cdf7370 this complicates maintenance. Started to clean up in jenkinsci/cloudbees-folder-plugin#353 + jenkinsci/cloudbees-folder-plugin#355 but to really remove the duplication it is simplest to just handle child deletion directly in AbstractItem.

Should have no behavioral effect for now since AbstractFolder overrides this, and the added block only pays attention to TopLevelItem, so should not affect MavenModuleSet or MatrixProject. jenkinsci/cloudbees-folder-plugin#356 completes the cleanup.

Testing done

JobTest.interruptOnDelete should check that non-folders are unaffected. Added a downstream test for what it is worth: jenkinsci/cloudbees-folder-plugin#356 (comment)

Proposed changelog entries

  • N/A

Before the changes are marked as ready-for-merge:

Maintainer checklist

@jglick jglick marked this pull request as ready for review October 25, 2023 23:23
// JENKINS-34939: do not hold the monitor on this folder while deleting them
// (thus we cannot do this inside performDelete)
try (ACLContext oldContext = ACL.as2(ACL.SYSTEM2)) {
for (Item i : ((ItemGroup<?>) this).getItems(TopLevelItem.class::isInstance)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

For now, retaining original behavior which is to say not calling delete() on children of non-top-level item groups (e.g., MatrixConfiguration). As in jenkinsci/cloudbees-folder-plugin#356 (comment), the only practical difference is likely to be in whether ItemListener is notified of these.

@NotMyFault NotMyFault added the skip-changelog Should not be shown in the changelog label Oct 26, 2023
@NotMyFault NotMyFault requested a review from a team October 26, 2023 20:43
@jglick jglick requested a review from Vlatombe October 27, 2023 12:31
Comment on lines +815 to +816
throw (AbortException) new AbortException(
"Failed to delete " + i.getFullDisplayName() + " : " + e.getMessage()).initCause(e);
Copy link
Member

@Vlatombe Vlatombe Nov 14, 2023

Choose a reason for hiding this comment

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

Looks like an opportunity to add missing constructor signatures to AbortException.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps. It is not normal to call AbortException.initCause, since the point of this class is that it suppresses a stack trace (including causes & suppressed). Whether the initCause was ever actually helpful in jenkinsci/cloudbees-folder-plugin#95, I am not sure; neither AbstractFolder.delete nor AbstractItem.performDelete throw this exception type, so it may be dead code.

@timja
Copy link
Member

timja commented Nov 14, 2023

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Nov 14, 2023
@NotMyFault NotMyFault merged commit 0297870 into jenkinsci:master Nov 16, 2023
16 checks passed
@jglick jglick deleted the AbstractFolder.delete branch November 16, 2023 23:06
jglick added a commit to jglick/cloudbees-folder-plugin that referenced this pull request Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants