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

Make the plugin JCasC compliant #2

Merged
merged 3 commits into from
Dec 19, 2019

Conversation

Wadeck
Copy link
Collaborator

@Wadeck Wadeck commented Dec 17, 2019

The problems were:

  • Some getters were not following basic POJO rules
  • The PostConstruct on private method is supported by the JavaDoc of the annotation but not yet by JCasC

The first point resulted in the plugin not being fully configurable and the second one to crash the instance once setup.

@daniel-beck @ewelinawilkosz

Copy link

@jeffret-b jeffret-b left a comment

Choose a reason for hiding this comment

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

Changes look fine and look like they should correct the problem. I haven't tested it out but I could do that if requested.

Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

Looking over it, this looks reasonable. The renames seem a bit unnecessary, but if it reads better in JCasC YAML…

Ideally we'd get someone from JCasC to confirm this variant is better, and the setup problem is addressed.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Would be nice to have a JIRA issue so that it is listed on the compatibility dashboard

@Wadeck
Copy link
Collaborator Author

Wadeck commented Dec 18, 2019

The renames seem a bit unnecessary

An attribute is "valid" for JCasC only if it has a field and a valid related getter, it was not the case due to the "ing" I added for some names :'(

Ideally we'd get someone from JCasC to confirm this variant is better, and the setup problem is addressed.

Anyone can test it with JCasC plugin, no need to be an expert actually.

But I totally agree we need at least one manual test to ensure it's working (in additon to mine), as we have 3 "look", they are not sufficient ;)

@oleg-nenashev => https://issues.jenkins-ci.org/browse/JENKINS-60523 with jcasc-compabitility label, not sure if you need something else in the ticket ;)

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Dec 18, 2019 via email

@ewelinawilkosz
Copy link

wanted to try but building make-compatible-with-jcasc branch (mvn install) fails for me:

[INFO] --- spotbugs-maven-plugin:3.1.11:check (spotbugs) @ strict-crumb-issuer ---
[INFO] BugInstance size is 1
[INFO] Error size is 0
[INFO] Total bugs: 1
[ERROR] StrictCrumbIssuer.md not guarded against concurrent access; locked 75% of time [org.jenkinsci.plugins.strictcrumbissuer.StrictCrumbIssuer, org.jenkinsci.plugins.strictcrumbissuer.StrictCrumbIssuer, org.jenkinsci.plugins.strictcrumbissuer.StrictCrumbIssuer, org.jenkinsci.plugins.strictcrumbissuer.StrictCrumbIssuer] Unsynchronized access at StrictCrumbIssuer.java:[line 191]Synchronized access at StrictCrumbIssuer.java:[line 276]Synchronized access at StrictCrumbIssuer.java:[line 440]Synchronized access at StrictCrumbIssuer.java:[line 441] IS_FIELD_NOT_GUARDED

Any idea what am I doing wrong? :)

@Wadeck
Copy link
Collaborator Author

Wadeck commented Dec 18, 2019

@ewelinawilkosz I just added the Jenkinsfile in this PR and it was failing for the same reason. I changed the code :)

@ewelinawilkosz
Copy link

Nice! That was fast :) I mean solving the whole compatibility issue. Thanks!
I managed to build and verify the behavior, works well

@Wadeck Wadeck merged commit 003744c into jenkinsci:master Dec 19, 2019
Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

belated LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants