prepare for JCasC compatibility #110

jetersen · 2019-10-24T05:01:09Z

update parent pom to latest v3.51
set java level to 8.
target Jenkins 2.60.3 in preparation for adding JCasC compatibility
fix upper bound dependency between credential plugins and ssh slaves plugin
json-lib is coming from Jenkins core.
remove custom build steps as they are no longer necessary due to pom update
and spotbugs.skip is available
fix credential deprecation warnings
fix incompatible type after jenknis 2.60.3 and avoid raw types
fix tabs/indents in pom 😭

- update parent pom to latest v3.51 - set java level to 8. - target Jenkins 2.60.3 in preparation for adding JCasC compatibility - fix upper bound dependency between credential plugins and ssh slaves plugin - json-lib is coming from Jenkins core. - remove custom build steps as they are no longer necessary due to pom update and spotbugs.skip is available

jetersen · 2019-10-24T05:08:10Z

Jenkins says NO, need to add the DescriptorVisibilityFilterImpl from #100 😄

https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fvsphere-cloud-plugin/detail/PR-110/1/pipeline/#step-31-log-44

jetersen · 2019-10-24T05:15:13Z

Here is the code related to visibility

        /**
         * Returns the list of {@link ComputerLauncher} descriptors appropriate to the supplied {@link Slave}.
         *
         * @param it the {@link Slave} or {@code null} to assume the slave is of type {@link #clazz}.
         * @return the filtered list
         * @since 2.12
         */
        @Nonnull
        @Restricted(NoExternalUse.class) // intended for use by Jelly EL only (plus hack in DelegatingComputerLauncher)
        public final List<Descriptor<ComputerLauncher>> computerLauncherDescriptors(@CheckForNull Slave it) {
            DescriptorExtensionList<ComputerLauncher, Descriptor<ComputerLauncher>> all =
                    Jenkins.getInstance().<ComputerLauncher, Descriptor<ComputerLauncher>>getDescriptorList(
                            ComputerLauncher.class);
            return it == null ? DescriptorVisibilityFilter.applyType(clazz, all)
                    : DescriptorVisibilityFilter.apply(it, all);
        }

https://github.com/jenkinsci/jenkins/blob/81e52b83d5caefd9a9637b47787baf9cfddb0421/core/src/main/java/hudson/model/Slave.java#L544-L559

Does the same as what

vsphere-cloud-plugin/src/main/java/org/jenkinsci/plugins/vSphereCloudProvisionedSlave.java

Lines 137 to 145 in 8e39d60

    
           public List<Descriptor<ComputerLauncher>> getComputerLauncherDescriptors() { 
        
               List<Descriptor<ComputerLauncher>> result = new ArrayList<Descriptor<ComputerLauncher>>(); 
        
               for (Descriptor<ComputerLauncher> launcher : Functions.getComputerLauncherDescriptors()) { 
        
                   if (!vSphereCloudLauncher.class.isAssignableFrom(launcher.clazz)) { 
        
                       result.add(launcher); 
        
                   } 
        
               } 
        
               return result; 
        
           }

jetersen · 2019-10-24T05:29:41Z

Okay green build 👍

Should I apply the DescriptorVisibilityFilter to the other methods here or do you want a separate PR for that?

pjdarton

I'm not 100% convinced that the DescriptorVisibilityFilterImpl classes are correct - can you verify?

src/main/java/org/jenkinsci/plugins/vSphereCloudSlave.java

pom.xml

src/main/java/org/jenkinsci/plugins/vSphereCloudSlave.java

src/main/java/org/jenkinsci/plugins/vSphereCloudSlaveTemplate.java

src/main/java/org/jenkinsci/plugins/vsphere/VSphereConnectionConfig.java

pjdarton · 2019-10-24T13:13:17Z

Thanks for getting back on this :-)

FYI my "at work" workload is currently such that I don't have the time to deep delve into this and learn enough to offer informed-and-instructive opinion so I guess you're going to be the subject-matter-expert on this without much help from me (although, if you can point me at specific documentation to improve my understanding, that'd be appreciated).
...also FYI, since you originally submitted #100, I added some contribution guidelines which I'd suggest you check over (I don't think you'll find any surprises).

Re: the one-or-many-PRs question, I'm a bit torn between "do it all for consistency" vs "cautious iterative changes"... but anything involving @Extension and/or Descriptor code always makes me uncomfortable (as I don't understand it nearly as well as I'd like) so in this case, caution wins.
i.e. I think that we should keep this PR as the minimal set of changes required to update to Java 8, parent pom etc, and follow up with one (or more) PR(s) for other changes.

After this PR, I think that, as long as you're willing to do the necessary tests to ensure that "it's all good" then we can do it in as many or as few PRs as you want. i.e. if you're driving it, it's up to you. I'll just push the "merge" button when you say.

However, this is all contingent on (you) doing some manual tests (or you writing unit tests) to convince me that it not only works in isolation but also continues to work "as intended" when there are other plugins contributing other @Extension options. If I have to do much testing myself then you'll have to wait a fair while before anything gets merged because, right now, I don't have much time for any of that.
i.e. it has to be tested before merger, and I'd like you to do that.

As far as testing goes, we don't want vSphere-specific launchers being suggested as an option for non-vsphere slaves, so that either means a unit-test (which would be the preferred option) or it means manual testing (typically by running up a test Jenkins with this plugin and a few others and checking that they're not interfering with each other in unexpected ways).

So, overall, I think we need to build up a list of Descriptors this code change might affect, and define some test cases for how to test that it's working (a) as before, (b) without confusing other plugins and (c) without being confused by other plugins, and then make sure that those test cases are either unit-tested or manually-tested before we merge.
That, obviously, starts with the descriptor(s) affected by this PR...

TL;DR: Convince me that this PR is all fine and I'll merge it; I don't want to have to test it myself 😛

jetersen · 2019-11-05T08:46:26Z

👋 @pjdarton

pjdarton · 2019-11-05T14:21:54Z

I'm not deliberately ignoring you ... work's been a tad busy.
My remaining concern is one of testing - we need to ensure that none of these changes has an adverse affect on the ability to configure new vSphere slaves (either cloud or static), and you previously indicated that you've not tried that (that's how the unintended IDE changes went unnoticed) so, while I'm sure that the code changes don't break anything for your use-case, it might break things for a wider audience.

i.e. either you test it (see my previous comment), or you'll have to wait for me to test it.

jetersen · 2019-11-05T15:16:04Z

I have tested it. We are using the updated code in production.

Again "not tested" part was in the attempt of splitting the PR in two #100 and #110 as you initially requested.

jetersen · 2019-11-05T15:17:59Z

I'd like to consume the incremental build (I would need to add a commit to enable incremental repo publishing)

jetersen · 2019-11-05T15:36:09Z

incremental allows me to do this in our production setup

diff --git a/dockerfile b/dockerfile
index b6022ed..26a139d 100644
--- a/dockerfile
+++ b/dockerfile
@@ -30,9 +30,7 @@ COPY --chown=jenkins plugins.txt $jenkins_refdir/

 # plugin installation layer, plugins are redownloaded everytime when plugins.txt is changed
 RUN mkdir -p $jenkins_refdir/plugins && \
-    install-plugins.sh < $jenkins_refdir/plugins.txt && \
-    curl -sSfL $artifactory/maven/org/jenkins-ci/plugins/vsphere-cloud/2.21-SNAPSHOT/vsphere-cloud-2.21-20191104.103446-1.hpi \
-    -o $jenkins_refdir/plugins/vsphere-cloud.jpi.override
+    install-plugins.sh < $jenkins_refdir/plugins.txt

 ARG CONTAINER_BUILD_TAG
 ENV CONTAINER_BUILD_TAG=$CONTAINER_BUILD_TAG
diff --git a/plugins.txt b/plugins.txt
index 40d1dd9..ddc27d7 100644
--- a/plugins.txt
+++ b/plugins.txt
@@ -132,8 +132,7 @@ token-macro:2.10
 trilead-api:1.0.5
 variant:1.3
 view-job-filters:2.1.1
# PR in progress: https://github.com/jenkinsci/vsphere-cloud-plugin/pull/100
-# vsphere-cloud:2.21-SNAPSHOT (private-d8485ff2-Joseph Petersen)
+vsphere-cloud:incrementals;org.jenkins-ci.plugins;2.21-rc447.075d17a3a216
 vstestrunner:1.0.8
 warnings-ng:7.1.0
 windows-azure-storage:1.1.2

pom.xml

pjdarton · 2019-11-05T16:13:40Z

If you hadn't snuck that incrementalify commit in, I'd've merged it just now ... but I think the pom is now slightly malformed with more than one value for the properties defined:

    <properties>
        <revision>2.22</revision>
        <changelist>-SNAPSHOT</changelist>
        <revision>2.21</revision>
        <changelist>-SNAPSHOT</changelist>
        <java.level>8</java.level>
        <jenkins.version>2.60.3</jenkins.version>
        <spotbugs.skip>true</spotbugs.skip>
    </properties>

Note: I would prefer incremental support to have been added as a separate PR so if it's just as easy to remove that commit from here and put it in a new PR then please do so. If not, it'll have to be fixed here and merging this will have to wait until that's fixed.

jetersen · 2019-11-05T16:24:21Z

I can't keep separating PRs... to do honest work 😭

This reverts commit 075d17a.

pjdarton · 2019-11-05T16:29:00Z

OK. Are you happy for me to merge what we've got here as of 25989eb ?

jetersen · 2019-11-05T16:30:02Z

yes please. I then need to add a new PR... that I then need #100 to be rebased on 😭

danjasmin · 2020-01-10T14:09:03Z

src/main/java/org/jenkinsci/plugins/vsphere/VSphereConnectionConfig.java

+            return new StandardListBoxModel().includeEmptyValue()
+                .includeMatching(Jenkins.getInstance(), StandardCredentials.class,
+                    Collections.singletonList(getDomainRequirement(vsHost)), CREDENTIALS_MATCHER);


With this change, I can no longer select any credentials since the dropdown list only contains “-none-“. I reverted the change to this method and the dropdown list is properly populated.
I would like to understand why the change was made and if the problem I am seeing is a bug or not. Any feedback you can supply would be appreciated.

Hmm.... my guess is that you don't have sufficient rights on your Jenkins server to see all the credentials you're used to seeing.
The old code specified

return new StandardListBoxModel().withEmptySelection().withMatching( CREDENTIALS_MATCHER, CredentialsProvider.lookupCredentials(StandardCredentials.class, instance, ACL.SYSTEM, getDomainRequirement(vsHost))

whereas the new code is missing the ACL.SYSTEM bit, so I would guess that it's showing you what credentials you're allowed to see instead of all the credentials the system can see.
...but that's a guess - I'm not hugely familiar with how StandardListBoxModel is (or should be) used so I can't tell the difference between "good enough that it'll work in my own local environment" and "actually correct".
I can tell you that it "works for me" - if I go to "manage jenkins" -> "configure system", scroll down to "clouds" and then click on the drop-down list of credentials for a specific vSphere cloud then I see lots of credentials ... but I'm logged as "as administrator" and I don't have any restrictions on how my credentials are used. My assumption is that your setup differs, which is why it's not working for you.

If we can get to the bottom of this, I'd be quite happy to receive a PR that fixes it for you (as long as it doesn't break it for anyone else)...

I believe for this to work with folder credentials it might need some tweaking

Ah, yes, folders make everything vastly more complicated, and they're not something I have any experience with.

If you are able to try out different options ... try using the new code but where it says Jenkins.getInstance() put instead containingFolderOrNull!=null?containingFolderOrNull:Jenkins.getInstance()
That should then provide the folder as the "context" for the credentials list instead of the Jenkins administrator-only list.

...and if that works then I suspect that we'll need similar changes elsewhere too...

This change seems to be the cause of JENKINS-61131.
Can please a maintainer take a look at my proposed fix in #116?

Looked; merged.

@albers Given that you know enough on this subject to be able to code a fix ... what's your opinion on containingFolderOrNull!=null?containingFolderOrNull:Jenkins.getInstance() vs Jenkins.getInstance() ?
My own experience with credential lists is more "shooting in the dark" than actual informed opinion so if you know enough to suggest fixes you probably know more about how this should be done than I do - I'd welcome any informed suggestions, especially if it'll let us get it right this time - trial and error isn't an approach I like to use...

ContainingFolderOrNull is not the proper way of doing it.

From jenkins you can ask for the context using @AncestorInPath Item item

https://github.com/jenkinsci/git-plugin/blob/c0e8144da63e3c8ccea7f00f7438b6e70aac9b00/src/main/java/hudson/plugins/git/UserRemoteConfig.java#L99-L121

While I am familiar (as in, I've seen it used before) with @AncestorInPath Item item, I'm less certain about Tasks.getAuthenticationOf((Queue.Task) project); I suspect that the vSphere plugin is unlikely to encounter a Queue.Task and that this is more specific to the git plugin's use-case...

The existing vsphere-plugin code already obtains @AncestorInPath AbstractFolder<?> containingFolderOrNull and as AbstractFolder is instanceof Item I would expect containingFolderOrNull to be the same value we'd get if this code had requested @AncestorInPath Item item instead.
What I'm less certain of is exactly what to do with it...

jenkinsci#110 changed the population of the credentials selection to no longer use deprecated api. Credentials are now resolved in the context of Jenkins.getAuthentication(), which does not return any matches. According to the documentation of the credentials-plugin, interactive lookups on the main configuration page should be performed as ACL.SYSTEM.

jenkinsci#110 changed the population of the credentials selection to no longer use deprecated api. Credentials are now resolved in the context of Jenkins.getAuthentication(), which does not return any matches. According to the documentation of the credentials-plugin, interactive lookups on the main configuration page should be performed as ACL.SYSTEM. Signed-off-by: Harald Albers <[email protected]>

jetersen added 4 commits October 24, 2019 06:52

fix credential deprecation warnings

6dfd402

fix incompatible type after jenknis 2.60.3 and avoid raw types

ebe71d7

fix tabs/indents in pom 😭

74caaa8

jetersen mentioned this pull request Oct 24, 2019

[JENKINS-60029] - mark VSphereCloudRetentionStrategy as an extension #100

Merged

replace getComputerLauncherDescriptors with DescriptorVisibilityFilter

87cf286

pjdarton requested changes Oct 24, 2019

View reviewed changes

jetersen added 3 commits October 24, 2019 17:26

fix copy pasta error, thanks IDEA 😆

a4a83b3

use for each loop instead of stream api

818d48d

fewer line changes

590bd1f

incrementalify

075d17a

pjdarton reviewed Nov 5, 2019

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

Revert "incrementalify"

25989eb

This reverts commit 075d17a.

pjdarton merged commit 35f498f into jenkinsci:master Nov 5, 2019

jetersen deleted the pom branch November 5, 2019 16:40

danjasmin reviewed Jan 10, 2020

View reviewed changes

albers mentioned this pull request Mar 3, 2020

[JENKINS-61131] Fix broken credentials selection #116

Merged

pjdarton mentioned this pull request Aug 18, 2020

Fix Jenkins 61702 #118

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prepare for JCasC compatibility #110

prepare for JCasC compatibility #110

jetersen commented Oct 24, 2019

jetersen commented Oct 24, 2019

jetersen commented Oct 24, 2019

jetersen commented Oct 24, 2019

pjdarton left a comment

pjdarton commented Oct 24, 2019

jetersen commented Nov 5, 2019

pjdarton commented Nov 5, 2019

jetersen commented Nov 5, 2019 •

edited

Loading

jetersen commented Nov 5, 2019

jetersen commented Nov 5, 2019 •

edited

Loading

pjdarton commented Nov 5, 2019

jetersen commented Nov 5, 2019

pjdarton commented Nov 5, 2019

jetersen commented Nov 5, 2019

danjasmin Jan 10, 2020

pjdarton Jan 10, 2020

jetersen Jan 10, 2020

pjdarton Jan 10, 2020 •

edited

Loading

albers Mar 6, 2020

pjdarton Mar 10, 2020

jetersen Mar 10, 2020

pjdarton Mar 10, 2020

prepare for JCasC compatibility #110

prepare for JCasC compatibility #110

Conversation

jetersen commented Oct 24, 2019

jetersen commented Oct 24, 2019

jetersen commented Oct 24, 2019

jetersen commented Oct 24, 2019

pjdarton left a comment

Choose a reason for hiding this comment

pjdarton commented Oct 24, 2019

jetersen commented Nov 5, 2019

pjdarton commented Nov 5, 2019

jetersen commented Nov 5, 2019 • edited Loading

jetersen commented Nov 5, 2019

jetersen commented Nov 5, 2019 • edited Loading

pjdarton commented Nov 5, 2019

jetersen commented Nov 5, 2019

pjdarton commented Nov 5, 2019

jetersen commented Nov 5, 2019

danjasmin Jan 10, 2020

Choose a reason for hiding this comment

pjdarton Jan 10, 2020

Choose a reason for hiding this comment

jetersen Jan 10, 2020

Choose a reason for hiding this comment

pjdarton Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

albers Mar 6, 2020

Choose a reason for hiding this comment

pjdarton Mar 10, 2020

Choose a reason for hiding this comment

jetersen Mar 10, 2020

Choose a reason for hiding this comment

pjdarton Mar 10, 2020

Choose a reason for hiding this comment

jetersen commented Nov 5, 2019 •

edited

Loading

jetersen commented Nov 5, 2019 •

edited

Loading

pjdarton Jan 10, 2020 •

edited

Loading