Fix serverAuthenticationToken being required for configuration via CasC #124

Smasherr · 2019-08-26T16:08:09Z

This fix avoids the following error message

io.jenkins.plugins.casc.ConfiguratorException: serverAuthenticationToken is required to configure class hudson.plugins.sonar.SonarInstallation

when using a YAML configuration without serverAuthenticationToken

henryju

LGTM

src/test/java/hudson/plugins/sonar/CasCTest.java

oleg-nenashev · 2019-08-27T09:45:05Z

CC jenkinsci/configuration-as-code-plugin#809

oleg-nenashev

It is up to plugin maintainers, but from what I see it breaks binary compatibility while it could be avoided

src/main/java/hudson/plugins/sonar/SonarInstallation.java

src/test/java/hudson/plugins/sonar/CasCTest.java

Smasherr · 2019-08-27T10:53:57Z

Should be fixed in CasC jenkinsci/configuration-as-code-plugin#1025

thorin · 2019-09-28T20:27:51Z

@Smasherr jenkinsci/configuration-as-code-plugin#1025 has been merged.

But now @Nullable should be replaced with @CheckForNull on SonarInstallation to fix the configuration via CasC.

Any reason to prefer @Nullable over @CheckForNull ?

Smasherr · 2019-09-29T15:08:42Z

@oleg-nenashev Please review the latest state of this PR. Would you also be so kind to answer this?

Any reason to prefer @Nullable over @CheckForNull?

Smasherr · 2019-10-05T10:21:05Z

Bump: any feedback?

timja · 2019-10-05T14:02:43Z

@oleg-nenashev Please review the latest state of this PR. Would you also be so kind to answer this?

Any reason to prefer @Nullable over @CheckForNull?

CheckForNull is normally preferred as it has better integration into static code analysis frameworks like spotbugs AFAIK

pom.xml

src/test/java/hudson/plugins/sonar/CasCTest.java

Co-Authored-By: Tim Jacomb <[email protected]>

timja

LGTM @henryju?

timja · 2019-10-05T14:38:46Z

The build is failing because it's untrusted I assume:
/home/travis/.local/bin/install: line 19: ARTIFACTORY_URL: unbound variable

henryju · 2019-10-07T07:45:25Z

@dbmeneses can you please take over this PR?

Smasherr · 2019-10-14T08:44:13Z

Bump: merge it please

timja · 2019-10-23T08:16:59Z

@Smasherr can you please resolve the conflicts?

timja · 2019-10-23T08:30:00Z

Would be cool to have this merged first: #130

probably no harm in cherry-picking that into this PR as well

timja · 2019-10-23T09:11:03Z

@henryju / @dbmeneses could we get this reviewed / merged please? We have users asking about this often

wouter-admiraal-sonarsource

Hi @Smasherr and @timja ,

Sorry this is taking a bit long. Let me do a final check. In the meantime, I added 2 small nitpicks about the code format.

src/test/resources/hudson/plugins/sonar/casc/configuration-as-code.yaml

wouter-admiraal-sonarsource · 2019-10-25T10:04:57Z

src/test/java/hudson/plugins/sonar/casc/CasCTest.java

+    protected String stringInLogExpected() {
+        return "credentialsId = test-id";
+    }
+}


I see we forgot to link to our contribution guidelines for this project. Sorry about that.

Could you please reformat this code so it follows our code style (config for Eclipse and IntelliJ available here)? From the looks of it, I think only the indentation should be corrected.

I see we forgot to link to our contribution guidelines for this project. Sorry about that.

Could you please reformat this code so it follows our code style (config for Eclipse and IntelliJ available here)? From the looks of it, I think only the indentation should be corrected.

Checkstyle? 🙂

henryju · 2019-10-25T10:15:52Z

I looked again and seems fine to me. I'm just concerned about the use of @CheckForNull. In SonarSource we tend to use @Nullable. Is the meaning the same for CasC?

timja · 2019-10-25T11:27:48Z

Nullable

See the discussion here, about CheckForNull and Nullable.
jenkinsci/configuration-as-code-plugin#1053 (comment)

The jenkins project tends to prefer CheckForNull as it has better integration with spotbugs. i.e. you must check a value for null if it could be null

henryju · 2019-10-25T11:58:58Z

We are not using Spotbugs but SonarQube ;) From CasC point of view, does it make a difference if we switch to @Nullable?

timja · 2019-10-25T12:06:14Z

We are not using Spotbugs but SonarQube ;) From CasC point of view, does it make a difference if we switch to @Nullable?

Needs a code change in JCasC and a release, Oleg didn't seem terribly keen on Nullable but probably no big deal..

Smasherr · 2019-10-28T19:10:19Z

Hey guys, nice to see finally some actions here. I've been on vacation and can do the requested changes to this PR tomorrow.

…code.yaml Co-Authored-By: Tim Jacomb <[email protected]>

Smasherr · 2019-10-29T06:55:15Z

@wouter-admiraal-sonarsource Hi, does it look better to you now?

wouter-admiraal-sonarsource · 2019-10-29T08:51:35Z

@wouter-admiraal-sonarsource Hi, does it look better to you now?

Perfect, thanks. I'll run the QA one more time from #132, and we'll finally merge. Thank you for your patience and contribution 👍 !

pom.xml

Smasherr · 2019-10-29T09:41:25Z

@wouter-admiraal-sonarsource You're welcome! Would you be so kind and to do a release with this change anytime soon?

Smasherr · 2019-11-04T13:56:32Z

Status?

nettworks-tooling · 2019-11-04T14:39:41Z

Any news on this topic? Would be very cool if this would be merged soon as we spot issues at this point on our automated Jenkins master setup.

wouter-admiraal-sonarsource · 2019-11-06T09:06:50Z

This was merged in #132. Thank you all for your help 👍

Smasherr · 2019-11-06T09:13:07Z

@wouter-admiraal-sonarsource good news, thanks! Now it would be great to have this in a release any time soon :-)

Wurden · 2019-11-21T15:43:19Z

I would like to add that it not only fails when serverAuthenticationToken is missing, it fails if any possible config parameter is missing, so for a working JCasC you need to include everything even though values are empty. Most other plugins have defaults and do not fail when config parameters are empty.
Just for reference, see below for working configurations. Hope this fix gets released soon.

sonarGlobalConfiguration:
  installations:
    - name: "my-sonarqube"
      serverUrl: "https://url-to-sonarqube.com/"
      serverAuthenticationToken: "" # Although we don't need any of below parameters, the Sonar plugin requires them to be present
      credentialsId: ""
      webhookSecretId: ""
      mojoVersion: ""
      additionalProperties: ""
      additionalAnalysisProperties: ""
      triggers:
        envVar: ""
        skipScmCause: false
        skipUpstreamCause: false

The same is the case for the sonarRunnerInstallation config, you also need to include everything there, even if empty.

tool:
  sonarRunnerInstallation:
    installations:
    - name: "my-sonarqube"
      home: "" # Sonar plugin requires home to be present as well
      properties:
      - installSource:
          installers:
          - sonarRunnerInstaller:
              id: "4.2.0.1873"

Smasherr · 2019-11-21T16:21:46Z

@Wurden The result of this change is that only 3 parameters remain required: name, serverUrl and triggers. The latter became optional in #137, so now only name and serverUrl are mandatory.

henryju approved these changes Aug 27, 2019

View reviewed changes

src/test/java/hudson/plugins/sonar/CasCTest.java Outdated Show resolved Hide resolved

Smasherr force-pushed the master branch 2 times, most recently from e8a5f7b to 22fd288 Compare August 27, 2019 09:34

oleg-nenashev reviewed Aug 27, 2019

View reviewed changes

src/main/java/hudson/plugins/sonar/SonarInstallation.java Show resolved Hide resolved

src/test/java/hudson/plugins/sonar/CasCTest.java Outdated Show resolved Hide resolved

oleg-nenashev mentioned this pull request Aug 27, 2019

DataBoundConfigurator enforces Constructor fields when ParametersAreNonnullByDefault is set, regardless of the CheckForNull flag jenkinsci/configuration-as-code-plugin#1025

Closed

Smasherr force-pushed the master branch from 22fd288 to eb7ba40 Compare August 27, 2019 10:39

Smasherr closed this Aug 27, 2019

Smasherr reopened this Sep 29, 2019

Smasherr force-pushed the master branch from eb7ba40 to d1a6640 Compare September 29, 2019 13:55

Fix serverAuthenticationToken being required for configuration via CasC

d1a6640

Turn TriggersConfig:envVar optional

519be3a

ghost assigned henryju Sep 30, 2019

Turn mojoVersion optional, fix build, adjust test

cddd1a1

timja reviewed Oct 5, 2019

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

timja reviewed Oct 5, 2019

View reviewed changes

src/test/java/hudson/plugins/sonar/CasCTest.java Outdated Show resolved Hide resolved

Smasherr force-pushed the master branch from aceeae6 to 4bb7158 Compare October 5, 2019 14:09

Smasherr and others added 2 commits October 5, 2019 16:09

Improve the comment in the pom.xml

4bb7158

Co-Authored-By: Tim Jacomb <[email protected]>

Convention over configuration

d728f54

timja approved these changes Oct 5, 2019

View reviewed changes

Smasherr added 2 commits October 10, 2019 14:14

Update JCasC to 1.32

962ee56

Mark sonarRunnerInstallation->home as optional

4b1ae0c

timja mentioned this pull request Oct 14, 2019

How to configure sonarqube plugin? jenkinsci/configuration-as-code-plugin#1159

Closed

3 tasks

Merge branch 'master' into master

9448c8c

wouter-admiraal-sonarsource reviewed Oct 25, 2019

View reviewed changes

wouter-admiraal-sonarsource assigned wouter-admiraal-sonarsource and unassigned henryju Oct 25, 2019

wouter-admiraal-sonarsource mentioned this pull request Oct 25, 2019

Internal PR for #124 #132

Merged

Smasherr and others added 2 commits October 29, 2019 07:24

Update src/test/resources/hudson/plugins/sonar/casc/configuration-as-…

1b1787a

…code.yaml Co-Authored-By: Tim Jacomb <[email protected]>

Format according to sonarqube code and import order guidelines

1f9140b

henryju reviewed Oct 29, 2019

View reviewed changes

pom.xml Show resolved Hide resolved

wouter-admiraal-sonarsource closed this Nov 6, 2019

Smasherr mentioned this pull request Nov 7, 2019

Fix triggers being required for configuration via CasC #136

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix serverAuthenticationToken being required for configuration via CasC #124

Fix serverAuthenticationToken being required for configuration via CasC #124

Smasherr commented Aug 26, 2019

henryju left a comment

oleg-nenashev commented Aug 27, 2019

oleg-nenashev left a comment

Smasherr commented Aug 27, 2019

thorin commented Sep 28, 2019 •

edited

Loading

Smasherr commented Sep 29, 2019 •

edited

Loading

Smasherr commented Oct 5, 2019

timja commented Oct 5, 2019

timja left a comment

timja commented Oct 5, 2019

henryju commented Oct 7, 2019

Smasherr commented Oct 14, 2019

timja commented Oct 23, 2019

timja commented Oct 23, 2019

timja commented Oct 23, 2019

wouter-admiraal-sonarsource left a comment

wouter-admiraal-sonarsource Oct 25, 2019

timja Oct 25, 2019

henryju commented Oct 25, 2019

timja commented Oct 25, 2019

henryju commented Oct 25, 2019

timja commented Oct 25, 2019

Smasherr commented Oct 28, 2019

Smasherr commented Oct 29, 2019

wouter-admiraal-sonarsource commented Oct 29, 2019

Smasherr commented Oct 29, 2019

Smasherr commented Nov 4, 2019

nettworks-tooling commented Nov 4, 2019

wouter-admiraal-sonarsource commented Nov 6, 2019

Smasherr commented Nov 6, 2019

Wurden commented Nov 21, 2019 •

edited

Loading

Smasherr commented Nov 21, 2019 •

edited

Loading

Fix serverAuthenticationToken being required for configuration via CasC #124

Fix serverAuthenticationToken being required for configuration via CasC #124

Conversation

Smasherr commented Aug 26, 2019

henryju left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Aug 27, 2019

oleg-nenashev left a comment

Choose a reason for hiding this comment

Smasherr commented Aug 27, 2019

thorin commented Sep 28, 2019 • edited Loading

Smasherr commented Sep 29, 2019 • edited Loading

Smasherr commented Oct 5, 2019

timja commented Oct 5, 2019

timja left a comment

Choose a reason for hiding this comment

timja commented Oct 5, 2019

henryju commented Oct 7, 2019

Smasherr commented Oct 14, 2019

timja commented Oct 23, 2019

timja commented Oct 23, 2019

timja commented Oct 23, 2019

wouter-admiraal-sonarsource left a comment

Choose a reason for hiding this comment

wouter-admiraal-sonarsource Oct 25, 2019

Choose a reason for hiding this comment

timja Oct 25, 2019

Choose a reason for hiding this comment

henryju commented Oct 25, 2019

timja commented Oct 25, 2019

henryju commented Oct 25, 2019

timja commented Oct 25, 2019

Smasherr commented Oct 28, 2019

Smasherr commented Oct 29, 2019

wouter-admiraal-sonarsource commented Oct 29, 2019

Smasherr commented Oct 29, 2019

Smasherr commented Nov 4, 2019

nettworks-tooling commented Nov 4, 2019

wouter-admiraal-sonarsource commented Nov 6, 2019

Smasherr commented Nov 6, 2019

Wurden commented Nov 21, 2019 • edited Loading

Smasherr commented Nov 21, 2019 • edited Loading

thorin commented Sep 28, 2019 •

edited

Loading

Smasherr commented Sep 29, 2019 •

edited

Loading

Wurden commented Nov 21, 2019 •

edited

Loading

Smasherr commented Nov 21, 2019 •

edited

Loading