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

[JENKINS-45055] Fix all spotbugs #149

Closed
wants to merge 6 commits into from

Conversation

res0nance
Copy link

Fix all remaining spotbugs in envinject

@oleg-nenashev oleg-nenashev self-requested a review October 7, 2019 07:05
@res0nance
Copy link
Author

I'm kinda stumped that this broke a test and because my machine refuses to run the tests I'm pretty much stuck

@res0nance
Copy link
Author

Apparently the older version of script security just doesn't run right on my system and updating made the test run :/

@CheckForNull
private final String scriptFilePath;
@CheckForNull
private final String scriptContent;
@CheckForNull
@SuppressFBWarnings(value = "SE_BAD_FIELD", justification = "EnvInjectEvaluatedGroovyScriptTest#testWorkaroundSecurity86 relies on the serialization of this object")
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why we can't serialize this bit without Spotbugs complaining

@@ -49,7 +49,7 @@ public Environment setUp(AbstractBuild build, Launcher launcher, BuildListener l
public static final class DescriptorImpl extends BuildWrapperDescriptor {
@Override
public String getDisplayName() {
return null;
return "SetEnvBuildWrapper";
Copy link
Member

Choose a reason for hiding this comment

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

If you put a name, these wrappers will start appearing in the lists IIRC

Copy link
Author

Choose a reason for hiding this comment

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

Hmm i guess i could set it to a blank name so spotbugs wont complain, would that be ok?

@oleg-nenashev
Copy link
Member

Encoding and display name changes may cause regressions. I still need to review it carefully, not including into the today;s release.

@res0nance res0nance closed this by deleting the head repository Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants