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

Add support for the folders plugin #36

Merged
merged 15 commits into from
Nov 24, 2015
Merged

Add support for the folders plugin #36

merged 15 commits into from
Nov 24, 2015

Conversation

s0undt3ch
Copy link
Contributor

No description provided.

@s0undt3ch
Copy link
Contributor Author

This initial build will fail because of the jenkins version bump.
I just want to see what's broken with this update and if my changes add any new failures.

@s0undt3ch
Copy link
Contributor Author

The actual folders support will come next

@jenkinsadmin
Copy link
Member

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

@s0undt3ch
Copy link
Contributor Author

No new failures were added from my code addition.

The test failures are related to the minimum jenkins version bump. Unfortunately, I'm not a java developer and I'm really not familiar with Java or even Jenkins unit testing framework to fix the failing issues. Help from someone with a depth knowledge of it would be greatly appreciated in order to get this into the next release.

I am using my code changes in a Jenkins instance and it's working as supposed.

@rsandell
Copy link
Member

rsandell commented May 4, 2015

When I started to dig into fixing the unit tests I noticed that some of BuildLogIndicationTest is failing because your algorithm doesn't handle when jenkins is running in a different context root, like it does when running the tests on newer versions of core.

@s0undt3ch
Copy link
Contributor Author

So, it fails when Jenkins is served from /<whatever right?

@rsandell
Copy link
Member

rsandell commented May 4, 2015

Yes,
I started to make some fixes, by first bumping the dependency to 1.580.1 and there I noticed that when the test ran jenkins was running on http://localhost:54545/jenkins/ so urlParts[0] became /jenkins so fullFolderName became //jenkins and that made getItemInstance become null and an NPE then got thrown further down.

@s0undt3ch
Copy link
Contributor Author

Want me to try and address this issue in this PR, or are you intending on pushing your fixed tests with the bumped Jenkins version to the master branch(or any other branch) and I'll rebase and fix?

@rsandell
Copy link
Member

rsandell commented May 4, 2015

I stopped once I noticed that the algorithm was wrong, so I've only done pom changes and checkstyle fixes so far. But I can push the few things that I've done and you can pull that to your fork if you want to.

@s0undt3ch
Copy link
Contributor Author

I think that I've taken care of the existance(or not) of a URL prefix... Let's see what the tests say....

@rsandell
Copy link
Member

Seems like it needs some more tweaking ;)

@@ -57,7 +56,6 @@
/**
* @param pattern the String value.
*/
@DataBoundConstructor
Copy link
Member

Choose a reason for hiding this comment

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

why?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I no longer remember why... I can get that back in if desirable...

@s0undt3ch
Copy link
Contributor Author

As soon as the Jenkins version is bumped the failures start happening. I tried cherry picking your LTS commits into master to confirm this, and, the master branch with the Jenkins version bump failures seem exactly the same as those in this PR.

I am, by no means, a Java developer, so, much of this code was done by try and failing and reading other peoples code.

If I could get a hold on a branch where the Jenkins version is bumped and the don't tests fail, that would be way more simple for me to apply this PR against and know if the failures are from my code or not....

@s0undt3ch
Copy link
Contributor Author

Updated my previous comment(it was written on mobile and had some bad phrases in there)...

* that comes before the first '/job' occurrent which is either nothing or the
* prefix from where jenkins is served, ie: http://localhost/jenkins/job/<job>/<buildNumber>
*/
String interestingJobParts = urlParts[0].split("/job", 2)[-1];
Copy link
Member

Choose a reason for hiding this comment

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

-1 is not a valid array index

@rsandell
Copy link
Member

I have managed to fix most of the tests to some degree. What I found so far is that the failures in MultilineBuildLogIndicationTest and BuildLogIndicationTest are due to your changes, see inline comment above.

@rsandell
Copy link
Member

I fixed all other test failures except those failed to your change on the same branch as before

https://github.com/jenkinsci/build-failure-analyzer-plugin/tree/s0undt3ch-features/folders-support

s0undt3ch and others added 3 commits November 23, 2015 16:57
Just converted to JenkinsRule to see if that helped things
And fixed some checkstyle errors
@s0undt3ch
Copy link
Contributor Author

The reason for the data bound question...

/home/vampas/projects/jenkins/build-failure-analyzer-plugin/src/main/java/com/sonyericsson/jenkins/plugins/bfa/model/indication/Indication.java:[62,11] error: @DataBoundConstructor may not be used on an abstract class (only on concrete subclasses)

@s0undt3ch
Copy link
Contributor Author

I've cherry-picked your changes into [https://github.com/s0undt3ch/build-failure-analyzer-plugin/tree/features/bump-to-lts]. As you mentioned, the tests pass. Thank You!

I'll apply my feature on top of that.

@rsandell
Copy link
Member

Yes a simple look in the git history revealed that I was the one who removed it :)

You should be able to simply pull the branch instead if cherry picking, I pulled from yours and just added my commit. A cherry pick would duplicate that commit which might be unnessesary.

@s0undt3ch
Copy link
Contributor Author

Updated some code.....

Dam! It still fails...

Folder jobs must then have more that two, `/job` occurrences in the
first array element.
@s0undt3ch
Copy link
Contributor Author

The tests now pass but this is still not correct...

@s0undt3ch
Copy link
Contributor Author

The mongo tests don't fail locally. Closing and reopening to trigger a new build(if this is how it works)...

@s0undt3ch s0undt3ch closed this Nov 24, 2015
@s0undt3ch s0undt3ch reopened this Nov 24, 2015
@s0undt3ch
Copy link
Contributor Author

Ok, I have no idea why the mongo tests are failing(they aren't locally)... does this makes any sense to you @rsandell?

@rsandell
Copy link
Member

It could be a timezone issue. It's a bit cumbersome for me to pull down your changes since you did a cherry pick instead of a merge/pull, but I'll muster through it ;)

@s0undt3ch
Copy link
Contributor Author

@rsandell Sorry.... Want me to do a clean PR?

@rsandell
Copy link
Member

Nope, no worries. I got it running locally. It seems to be a jdk 8 vs jdk 7 issue.

Thanks for all your work!

rsandell added a commit that referenced this pull request Nov 24, 2015
@rsandell rsandell merged commit 1f0fa21 into jenkinsci:master Nov 24, 2015
@s0undt3ch
Copy link
Contributor Author

AWESOME!!!!

🎆

@s0undt3ch s0undt3ch deleted the features/folders-support branch November 24, 2015 15:57
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.

3 participants