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

Fixing JENKINS-20808 by escaping single quotes inside BUILDS_STATS #307

Merged
merged 1 commit into from Oct 12, 2017

Conversation

Projects
None yet
5 participants
@ssbarnea
Copy link
Contributor

commented Jan 19, 2017

This should fix https://issues.jenkins-ci.org/browse/JENKINS-20808 which was reported multiple times.

@ssbarnea ssbarnea force-pushed the ssbarnea:master branch 2 times, most recently from 76a6a3e to d1bcbba Jan 19, 2017

@ssbarnea

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2017

@rsandell Can you have a look at this? It should solve a bug that is more than 3 years old.

@rsandell

This comment has been minimized.

Copy link
Member

commented Jan 20, 2017

Well, the big question and why it probably hasn't been tackled before is if this also works on windows or if it will break even more.

@josselinchevalay

This comment has been minimized.

Copy link

commented Sep 5, 2017

hello,

@rsandell do you have an ETA when that can enable into plugin ?

Regards

@rsandell

This comment has been minimized.

Copy link
Member

commented Sep 12, 2017

Still waiting for a response to my inquiry.

@florent-vial

This comment has been minimized.

Copy link

commented Sep 19, 2017

@ssbarnea can you test it on windows?
This bug is annoying and creating a lot of confusion for developers.
I would expect > 90% users have their Jenkins server run on linux anyway

@ssbarnea

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2017

@florent-vial Sorry but the answer is no. I don't have a Windows machine, last time I had one was more than an year ago. It would just take too much time to deploy one. If someone can share one with Jenkins I would not mind helping.

@florent-vial

This comment has been minimized.

Copy link

commented Sep 19, 2017

@florent-vial

This comment has been minimized.

Copy link

commented Sep 19, 2017

I abandoned installing JDK8 on my windows 10 machine for building the plugin after reading this thread https://community.oracle.com/thread/3825840.
So i did the following:

  • downloaded the fix using:
    git clone https://github.com/jenkinsci/gerrit-trigger-plugin.git && cd gerrit-trigger-plugin && git fetch origin refs/pull/307/head:fix_quote && git checkout fix_quote

git log -1 shows
`commit d1bcbba (HEAD -> fix_quote)
Author: Sorin Sbarnea ssbarnea@redhat.com
Date: Thu Jan 19 13:37:40 2017 +0000

Fixing JENKINS-20808 by escaping single quotes inside BUILDS_STATS message.`
  • compiled the plugin on Ubuntu using java 1.8.0.121 mvn clean package. Result was:
    [INFO] ------------------------------------------------------------------------
    [INFO] BUILD SUCCESS
    [INFO] ------------------------------------------------------------------------
    [INFO] Total time: 29:18.291s
    [INFO] Finished at: Tue Sep 19 19:45:19 CEST 2017
    [INFO] Final Memory: 122M/5487M
    [INFO] ------------------------------------------------------------------------
  • install Jenkins 2.73.1 on my Windows 10 machine
  • install freshly compiled target/gerrit-trigger.hpi from above on my Windows Jenkins server
  • import 3 jobs triggering on changes in my usual repo on my usual Gerrit server
  • upload a change to my Gerrit server
  • observe that all 3 jobs are triggered properly in my Windows Jenkins server
  • observe that all 3 'Build Started" message are posted properly as comments in the Gerrit review
  • observe that the verify +1 verdict is posted properly in the Gerrit review.

@rsandell can you please consider merging that PL then?

@dsariel

This comment has been minimized.

Copy link

commented Oct 3, 2017

any news re this PR?

@rsandell rsandell merged commit d6474d7 into jenkinsci:master Oct 12, 2017

2 checks passed

Jenkins This pull request looks good
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.