-
Notifications
You must be signed in to change notification settings - Fork 175
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
Add spotbugs exclusions #235
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff is much longer than it needs to be, but if the formatting changes were reverted I think this would be good.
src/main/java/hudson/plugins/parameterizedtrigger/BlockableBuildTriggerConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/hudson/plugins/parameterizedtrigger/BlockableBuildTriggerConfig.java
Outdated
Show resolved
Hide resolved
https://github.com/jenkinsci/parameterized-trigger-plugin/pull/235/checks?check_run_id=5243312168 smells like a too-aggressive timeout? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
src/main/java/hudson/plugins/parameterizedtrigger/BlockableBuildTriggerConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/hudson/plugins/parameterizedtrigger/BlockableBuildTriggerConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/hudson/plugins/parameterizedtrigger/FileBuildTriggerConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/hudson/plugins/parameterizedtrigger/PredefinedPropertiesBuildTriggerConfig.java
Outdated
Show resolved
Hide resolved
I tried to see what the error was:
But now it is resolved, and all I did was undo the autoformatting done by IDE. I'm confused about what went wrong and how it got resolved... |
Probably just some flaky test unrelated to your PR. |
This is an attempt to see if the tests still pass with this variable being deleted.
src/main/java/hudson/plugins/parameterizedtrigger/FileBuildTriggerConfig.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Jesse Glick <[email protected]>
pom.xml
Outdated
@@ -217,7 +220,7 @@ | |||
<changelist>-SNAPSHOT</changelist> | |||
<gitHubRepo>jenkinsci/parameterized-trigger-plugin</gitHubRepo> | |||
<java.level>8</java.level> | |||
<jenkins.version>2.270</jenkins.version> | |||
<jenkins.version>2.289.1</jenkins.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as recommended here https://www.jenkins.io/doc/developer/plugin-development/choosing-jenkins-baseline/
can you change to later releases within an LTS line
? 2.289.3
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current PR matches the currently documented advice, but FWIW there is no real consensus here. See jenkins-infra/jenkins.io#4876
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comments!
Lmk if we mutually decide to change jenkins.version
be 2.289.3. I'm keeping it as 2.289.1 for now, unless Olivier has something to add.
@@ -20,6 +22,7 @@ | |||
|
|||
import static org.apache.commons.lang.StringUtils.isEmpty; | |||
|
|||
@SuppressFBWarnings(value="NM_SAME_SIMPLE_NAME_AS_SUPERCLASS", justification="Part of public API, reviewed all uses of Plugin are fully quantified in this class") | |||
public class Plugin extends hudson.Plugin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that extending Plugin
is deprecated.
parameterized-trigger-plugin/src/main/java/hudson/plugins/parameterizedtrigger/Plugin.java
Lines 31 to 32 in 5923a07
Stapler.CONVERT_UTILS.register(new EnumConverter(), | |
ResultCondition.class); |
@Initializer
, though it could probably simply be a static
initializer in ResultCondition.java
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood that extending Plugin
is deprecated.
Can you please help me in understanding what is our concern with Stapler.CONVERT_UTILS.register
and how would that be addressed by making it into an @Initializer
?
I read about CONVERT_UTILS
from its javadocs and since ResultCondition is an enum, maybe we are trying to register it so that it becomes easier for Stapler to bind the enum constants...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose so. I would have expected any enum
to be handled automatically by Stapler without a special call; maybe it is, and this call is just obsolete. Not sure offhand if it works to just move this to a static
block in ResultCondition
; I am guessing the key is whether ResultCondition
would already have been loaded for some other reason by the time someone saves a configure
page involving a value of this enum in some field and Jenkins starts processing a doSubmitConfigXml
method with a JSON payload. An @Initializer
is the more direct replacement and conservative fix, though it always runs whether or not you are even using this plugin. Whether this has any effective test coverage is another question (typically something calling configRoundtrip
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(To be clear, this is just an aside about an existing deprecated API, certainly not something that needs to be changed in this PR.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed info! This makes things little bit more clear to me now.
Minimum Jenkins version has been updated in other pull requests. Thanks for raising the concern. It has been resolved.
This PR simply updates the parent POM of this plugin from 4.31 to 4.33.