Support for programmatically generating OrganizationFolders #145

awittha · 2019-03-27T17:43:44Z

In my own life, I am building a Jenkins plugin that adds the ability to generate OrganizationFolders for a user against an SCM server (each of which will then generate MultiBranchProjects for each repo, as an OrganizationFolder does).

Suppress modify permissions in OrganizationFolder when it is a direct child of a ComputedFolder

The MultiBranchProject detects if it is a child of an OrganizationFolder and hides from users the ability to directly update and delete it, because it is managed programmatically by the ComputedFolder aspect of OrganizationFolder.

OrganizationFolder itself does not do this, so the automatically-generated organization folders can be modified and deleted by users after their creation. This isn't ideal.

I have taken the (above-linked) code block from MultiBranchProject that hides the modification permissions when it should, and added it to OrganizationFolder

I have made one change: Instead of checking for a specific implementation parent class, it will hide those permissions any time it is a child of a ComputedFolder - so anyone that computes & generates OrganizationFolders will get the correct behavior regardless of their specific implementation class' type.

I would have preferred to keep this entirely in my own plugin code by extending OrganizationFolder but it is a final class and so I must make the code change here.

I believe this is a low-risk contribution with value to all plugin authors, not just myself.

I don't know of anyone (or any plugin) that currently puts OrganizationFolders in ComputedFolders and expects human users to be able to modify & delete them through the WebUI. If any such users do exist, though, this change would break their workflow.

Do not use default MultiBranchProjectFactory(ies) onCreatedFromScratch if some exist

Currently, when an OrganizationFolder is created the onCreatedFromScratch() method will add all enabled-by-default MultiBranchProjectFactory types to the OrganizationFolder.

This updates that behavior to skip adding the factories if and only if there were already some factories set. This allows code that creates an OrganizationFolder to also specify which project factories the folder should use, and ensures that the OrganizationFolder won't go and add additional factories on its own.

I believe this is a low-risk contribution.

Only other workflows that were programmatically creating OrganizationFolders and relying on the default MultiBranchProjectFactory classes being added to the folders in addition to at least one non-default factory that the workflow added would experience a change in behavior.

src/main/java/jenkins/branch/OrganizationFolder.java

awittha · 2019-03-27T17:51:06Z

src/main/java/jenkins/branch/OrganizationFolder.java

+import javax.servlet.ServletException;
+
+import org.acegisecurity.AccessDeniedException;
+import org.acegisecurity.Authentication;


My IDE alphabetized imports.

org.acegisecurity.Authentication is an addition.

awittha · 2019-03-27T17:53:43Z

src/main/java/jenkins/branch/OrganizationFolder.java

@@ -695,7 +730,7 @@ public String getDescription() {
            );
        }

-        //@Override TODO once baseline is 2.x
+        @Override


This appears to have happened, so I did the TODO.

awittha · 2019-03-27T17:53:45Z

src/main/java/jenkins/branch/OrganizationFolder.java

@@ -676,7 +711,7 @@ public String getCategoryId() {
         *
         * @return A string with the description. {@code TopLevelItemDescriptor#getDescription()}.
         */
-        //@Override TODO once baseline is 2.x
+        @Override


This appears to have happened, so I did the TODO.

awittha · 2019-03-27T17:53:48Z

src/main/java/jenkins/branch/OrganizationFolder.java

@@ -665,7 +700,7 @@ public TopLevelItem newInstance(ItemGroup parent, String name) {
         *
         * @return A string with the category identifier. {@code TopLevelItemDescriptor#getCategoryId()}
         */
-        //@Override TODO once baseline is 2.x
+        @Override


This appears to have happened, so I did the TODO.

src/main/java/jenkins/branch/OrganizationFolder.java

bitwiseman · 2019-04-17T21:33:05Z

@awittha
Looks reasonable to me. Can you add some tests showing the behavior working?

… child of a ComputedFolder.

…d default-enabled MultiBranchProjectFactories if the projectFactories are empty. This allows OrganizationFolders' projectFactories to be set by code that creates them and avoids always adding enabled-by-default MultiBranchProjectFactories.

…iBranchProjectFactory implementations during OrganizationFolder#onCreatedFromScratch()

…when children of a ComputedFolder

awittha · 2019-04-19T18:21:22Z

Added unit tests & rebased off of latest master.

bitwiseman · 2019-04-25T15:04:49Z

src/test/java/jenkins/branch/OrganizationFolderTest.java

+                 * doesn't look like we can actually compute children during a unit test.
+                 * Normally we'd want to create the OrganizationFolder here
+                 * to match the "real" Jenkins workflow as closely as possible
+                 */


@awittha
Why can't we compute children during a unit test?

In my own code, when I want to kick off the computation of children of a ComputedFolder in a "safe" way, I used ComputedFolder#scheduleBuild(...).

This didn't actually result in any computations happening, during the unit tests - the actual computeChildren(...) method was never called.

I didn't attach a debugger, but my guess is that when it tries to actually schedule a build ( https://github.com/jenkinsci/cloudbees-folder-plugin/blob/master/src/main/java/com/cloudbees/hudson/plugins/folder/computed/ComputedFolder.java#L580 ), there isn't actually a build queue in the unit test's Jenkins "instance" (or maybe no executors...)?

In any case, it isn't terribly important to actually compute children as this code doesn't activate when an OrganizationFolder is created as part of a ComputedFolder computation, but merely when an OrganizationFolder's parent is a ComputedFolder - which we can set up without actually having to have a working ComputedFolder during the test.

Should I update the comment to be more specific to that effect - that it doesn't work out of the box and we don't really need it anyway - do you think?

If you want to add a little more to say why it is not a big deal, that'd be great.

Great work on this.

Updated the comment; new text:

* We don't actually need to compute children during a unit test. * We just need an OrganizationFolder to have this ComputedFolder * as its parent. * * Since item parents are set on the item when constructing it, * we can do that when we create the OrganizationFolder. * * Also, #scheduleBuild(...) to enqueue a computeChildren * doesn't work out-of-the-box in the current unit-test setup.

…he ComputedFolder in OrganizationFolderTest

bitwiseman · 2019-05-01T20:49:37Z

@reviewbybees Can I get another set of eyes on this it looks good to me, but I'd like a second upvote.

stephenc

Stick to the codes convention of imports order
MultibranchProject should use the ComputedFolder generalization too

stephenc · 2019-05-02T10:26:21Z

src/main/java/jenkins/branch/OrganizationFolder.java

@@ -24,7 +24,43 @@

 package jenkins.branch;

-import antlr.ANTLRException;
+import static jenkins.scm.api.SCMEvent.Type.CREATED;
+import static jenkins.scm.api.SCMEvent.Type.UPDATED;


static after non static

src/main/java/jenkins/branch/OrganizationFolder.java

src/test/java/jenkins/branch/OrganizationFolderTest.java

stephenc · 2019-05-02T15:23:57Z

@bitwiseman well given that it mirrors the exact change made in this PR, from an API perspective I would rather the two go hand in hand (it's just one line to change anyway)

stephenc

We have been badly burned by Guava, consequently any plugins for which I have any say are not using Guava. The rest is fine.

No need to seek re-approval once Guava is removed

src/test/java/jenkins/branch/OrganizationFolderTest.java

bitwiseman · 2019-05-06T21:02:54Z

@stephenc
You're three years too late.
667d51a

But okay, removed.

stephenc · 2019-05-06T21:12:33Z

Better late than never

stephenc

Once you figure out what’s up with the build failure

awittha · 2019-05-07T16:03:30Z

Thanks, y'all!

awittha force-pushed the computed-org-folder branch from e86e91e to 2dd56e1 Compare March 27, 2019 17:49

awittha commented Mar 27, 2019

View reviewed changes

src/main/java/jenkins/branch/OrganizationFolder.java Show resolved Hide resolved

awittha commented Mar 27, 2019

View reviewed changes

src/main/java/jenkins/branch/OrganizationFolder.java Outdated Show resolved Hide resolved

awittha commented Mar 27, 2019

View reviewed changes

awittha force-pushed the computed-org-folder branch 2 times, most recently from fb7ef20 to 4df7bef Compare March 27, 2019 17:53

awittha commented Mar 27, 2019

View reviewed changes

awittha changed the title ~~Suppress modify permissions in OrganizationFolder when it is a direct child of a ComputedFolder.~~ Support for programmatically generating OrganizationFolders Apr 5, 2019

awittha commented Apr 5, 2019

View reviewed changes

src/main/java/jenkins/branch/OrganizationFolder.java Show resolved Hide resolved

awittha commented Apr 5, 2019

View reviewed changes

src/main/java/jenkins/branch/OrganizationFolder.java Show resolved Hide resolved

awittha added 4 commits April 19, 2019 13:02

Suppress modify permissions in OrganizationFolder when it is a direct…

eafb15f

… child of a ComputedFolder.

[added] Unit tests for OrganizationFolder's behavior when adding Mult…

2144299

…iBranchProjectFactory implementations during OrganizationFolder#onCreatedFromScratch()

[added] unit tests for ACL filtering behavior of OrganizationFolders …

a54aad9

…when children of a ComputedFolder

awittha force-pushed the computed-org-folder branch from 6777159 to a54aad9 Compare April 19, 2019 18:02

bitwiseman reviewed Apr 25, 2019

View reviewed changes

bitwiseman approved these changes Apr 25, 2019

View reviewed changes

[updated] text explaining why we don't actually computeChildren for t…

302881f

…he ComputedFolder in OrganizationFolderTest

stephenc requested changes May 2, 2019

View reviewed changes

Modifications for feedback

3798c55

bitwiseman force-pushed the computed-org-folder branch from 4b2a813 to 3798c55 Compare May 2, 2019 16:25

Update pom.xml

5fc0d6f

stephenc requested changes May 2, 2019

View reviewed changes

src/test/java/jenkins/branch/OrganizationFolderTest.java Outdated Show resolved Hide resolved

stephenc approved these changes May 6, 2019

View reviewed changes

Remove ImmutableSet

5d50dcc

bitwiseman force-pushed the computed-org-folder branch from 6d8c8cb to 5d50dcc Compare May 6, 2019 21:38

bitwiseman merged commit 9ec330f into jenkinsci:master May 7, 2019

awittha deleted the computed-org-folder branch May 7, 2019 16:03

jglick mentioned this pull request Aug 21, 2020

Avoid TestingAuthenticationToken; use createDummySecurityRealm #209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for programmatically generating OrganizationFolders #145

Support for programmatically generating OrganizationFolders #145

awittha commented Mar 27, 2019 •

edited

Loading

awittha Mar 27, 2019

awittha Mar 27, 2019

awittha Mar 27, 2019

awittha Mar 27, 2019

bitwiseman commented Apr 17, 2019 •

edited

Loading

awittha commented Apr 19, 2019

bitwiseman Apr 25, 2019

awittha Apr 25, 2019 •

edited

Loading

bitwiseman Apr 25, 2019

awittha Apr 26, 2019

bitwiseman commented May 1, 2019

stephenc left a comment

stephenc May 2, 2019

stephenc commented May 2, 2019

stephenc left a comment

bitwiseman commented May 6, 2019

stephenc commented May 6, 2019

stephenc left a comment

awittha commented May 7, 2019

Support for programmatically generating OrganizationFolders #145

Support for programmatically generating OrganizationFolders #145

Conversation

awittha commented Mar 27, 2019 • edited Loading

Suppress modify permissions in OrganizationFolder when it is a direct child of a ComputedFolder

Do not use default MultiBranchProjectFactory(ies) onCreatedFromScratch if some exist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitwiseman commented Apr 17, 2019 • edited Loading

awittha commented Apr 19, 2019

Choose a reason for hiding this comment

awittha Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitwiseman commented May 1, 2019

stephenc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc commented May 2, 2019

stephenc left a comment

Choose a reason for hiding this comment

bitwiseman commented May 6, 2019

stephenc commented May 6, 2019

stephenc left a comment

Choose a reason for hiding this comment

awittha commented May 7, 2019

awittha commented Mar 27, 2019 •

edited

Loading

bitwiseman commented Apr 17, 2019 •

edited

Loading

awittha Apr 25, 2019 •

edited

Loading