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-33668] Adapt to new plugin parent POM and upgrade baseline to 1.580.1 #53

Merged
merged 2 commits into from Mar 28, 2016

Conversation

@andresrc
Copy link
Contributor

andresrc commented Jan 14, 2016

See JENKINS-33668 Adapt to new parent point and upgrade baseline to 1.580.1

  • Parent POM 2.3
  • Baseline 1.580.1
  • Adapt to new remoting interfaces.
  • Fix javadocs when building with Java 8.
  • Jelly XSS PI
  • Adapt to JTH 2.1
  • Additional findbugs fixes needed because of the baseline change.
  • Check build against 1.642.2 (PCT scenario).
@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented Jan 14, 2016

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

pom.xml Outdated
<project.build.outputEncoding>UTF-8</project.build.outputEncoding>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<maven.findbugs.failure.strict>true</maven.findbugs.failure.strict>
<jenkins.version>1.554.1</jenkins.version>

This comment has been minimized.

Copy link
@jglick

jglick Jan 19, 2016

Member

I would not advise even trying to use the new parent with anything older than 1.580.1; to start with, we would not be able to use jenkins-test-harness 2.x.

That said, this would be a fine time to increment the dependency IMHO. 1.554.x is now quite old.

pom.xml Outdated
<maven.findbugs.failure.strict>true</maven.findbugs.failure.strict>
<jenkins.version>1.554.1</jenkins.version>
<!-- TODO check if we'd better add the PI. -->
<jelly.requirePI>false</jelly.requirePI>

This comment has been minimized.

Copy link
@jglick

jglick Jan 19, 2016

Member

Yes you should add the PI. The rule is that all *.jelly should explicitly say they escape output by default; where you have something.jelly

${something}

and something might legitimately include HTML tags you want to render raw without risk of XSS (for example because it is coming from something.properties, or is a Java getter where the value is known not be “tainted” by user input), you can use

<j:out value="${something}"/>

By updating to a new parent POM which uses a new version of the HPI plugin that includes this InjectedTest, you confirm that you have reviewed all your views for XSS vulnerabilities.

@@ -239,7 +240,7 @@ public GetSlaveDigest(FilePath rootPath) {
public String call() {
StringBuilder result = new StringBuilder();
final File rootPath = new File(this.rootPathName);
for (File file : rootPath.listFiles()) {
for (File file : Preconditions.checkNotNull(rootPath.listFiles())) {

This comment has been minimized.

Copy link
@jglick

jglick Jan 19, 2016

Member

Throwing an IllegalArgumentException here is not really an improvement over a NullPointerException I think.

This comment has been minimized.

Copy link
@jglick

jglick Jan 19, 2016

Member

(ditto other places where File.listFiles or File.list is called)

@@ -144,6 +145,7 @@ public String call() {
/**
* * Using OperatingSystemMXBean, we can obtain the total number of open file descriptors.
*/
@IgnoreJRERequirement

This comment has been minimized.

Copy link
@jglick

jglick Jan 19, 2016

Member

Ought to be accompanied by a Java version check in GetUlimit, or at least catching LinkageError and reporting it gracefully.

public boolean accept(File dir, String name) {
return name.endsWith(".txt");
}
});
}));
for (File f : Util.fixNull(Arrays.asList(files))) {

This comment has been minimized.

Copy link
@jglick

jglick Jan 19, 2016

Member

Probably the call to Util.fixNull is bogus here. The null would be the return value of listFiles, not an element of the return value.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jan 19, 2016

Get #40 merged first (@oleg-nenashev ping), which should take care of the FB warnings, so you can focus on the POM.

@andresrc

This comment has been minimized.

Copy link
Contributor Author

andresrc commented Jan 20, 2016

Sure, I won't fail the build on findbugs error with a TODO to remove the switch when #40 is merged.

@andresrc andresrc force-pushed the andresrc:newParentPOM branch from 328153d to 6690f19 Jan 20, 2016
@andresrc andresrc changed the title [WiP] Experimental cleanup using new plugin parent POM [JENKINS-32495] [WiP] Test new experimental plugin parent POM Jan 20, 2016
@andresrc

This comment has been minimized.

Copy link
Contributor Author

andresrc commented Jan 20, 2016

The CI build error is because of findbugs. Expected. The build is not failed but marked unstable.

@andresrc andresrc force-pushed the andresrc:newParentPOM branch from 6690f19 to a4ac162 Feb 1, 2016
@andresrc andresrc changed the title [JENKINS-32495] [WiP] Test new experimental plugin parent POM [JENKINS-32493] Adapt to new plugin parent POM and upgrade baseline to 1.580.1 Feb 1, 2016
@andresrc

This comment has been minimized.

Copy link
Contributor Author

andresrc commented Feb 1, 2016

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Feb 1, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@andresrc

This comment has been minimized.

Copy link
Contributor Author

andresrc commented Feb 1, 2016

As before, failing CI is findbugs.

@christ66

This comment has been minimized.

Copy link
Member

christ66 commented Mar 2, 2016

@andresrc please rebase and there should be no more findbugs errors.

@andresrc andresrc force-pushed the andresrc:newParentPOM branch from a4ac162 to 3f366da Mar 8, 2016
@andresrc andresrc changed the title [JENKINS-32493] Adapt to new plugin parent POM and upgrade baseline to 1.580.1 [JENKINS-33668] Adapt to new plugin parent POM and upgrade baseline to 1.580.1 Mar 8, 2016
@andresrc

This comment has been minimized.

Copy link
Contributor Author

andresrc commented Mar 8, 2016

rebased

@reviewbybees

@christ66

This comment has been minimized.

Copy link
Member

christ66 commented Mar 14, 2016

🐝

@andresrc

This comment has been minimized.

Copy link
Contributor Author

andresrc commented Mar 28, 2016

@andresrc andresrc merged commit f231a0c into jenkinsci:master Mar 28, 2016
1 check passed
1 check passed
Jenkins This pull request looks good
Details
@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Mar 28, 2016

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

@andresrc andresrc deleted the andresrc:newParentPOM branch Mar 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.