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

support triggering for wip-state-changed and private-state-changed #383

Merged
merged 20 commits into from Feb 15, 2019

Conversation

Projects
None yet
5 participants
@cashlalala
Copy link
Contributor

commented Sep 19, 2018

No description provided.

cashlalala added some commits Sep 19, 2018

@cashlalala

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2018

@rsandell I'm going to fix the checkstyle errors but not sure about what caused the timeout errors in tests, would you give me some suggestions to fix that?

@sselberg

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

It's great that you started with this long overdue addition!

Some comments about this change and thoughts about how WIP and private state should be handled in Jenkins in general:

[NEEDS] Check if WIP/private is supported
You will need to add a check in:
src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/version/GerritVersionChecker.java
Since WIP and private are not supported with Gerrit versions <2.15.

[NEEDS] Exclude WIP|private changes
In order to work with WIP and private states in gerrit-trigger there also needs to be configuration to "Exclude WIP changes" and "Exclude Private changes" (analogous to the current "Exclude drafts"). Otherwise Jenkins will build all private and WIP changes which you don't necessarily want. (I was discussing with one of our users today that had a use-case where he wanted to start his own set of builds if he uploaded WIP changes and not once the changes were published which would call for an XOR configuration option. Perhaps it's not necessary to implement that at this stage but it may be worth considering).
As it currently stands you can trigger jobs on wip|private-state-changed but it's not necessarily useful. If you are concerned with any of these states you are most likely interested in triggering a build when the state transitions from private to non-private and/or when a change transitions state from WIP to non-WIP.

State change to private and or WIP when build is already started
Should there be a similar option as for "patchset-created" (where you can choose to stop builds of older patchsets if a new patchset is created) to stop ongoing builds if change is toggled into private or WIP state? There are downsides to such an option if you consider my next remark.

State toggling
There probably needs to be a warning (similar to the current warning about draft visibility, src/main/webapp/trigger/help-DraftPublished.html) explaining that unlike draft-published which could only happen once per patch-set (so normally less than let's say 3 times/change), wip|private-state-change can change as often as the Gerrit server can handle the state change request. So any user might press the "private" button repeatedly to see what it's for or some automation might run berserk and change the states several times per seconds, if you trigger on the state-change you potentially start and stop builds as often. I don't know if there should be a mechanism for setting this threshold or if users should handle it themselves or if it's so unlikely that it need not be dealt with at all. With manual manipulation I don't estimate that there would be any real consequences but that of course depends on the nature of the job, you might want to take it under consideration when configuring resource-heavy/long-running jobs.

Somewhat related concerns regarding drafts
Since drafts was removed in Gerrit v2.15 (at the same time that WIP and private was introduced) src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/version/GerritVersionChecker.java needs to be augumented to allow the features to specify a "highest version" so that all "drafts" related stuff is turned off if Gerrit version is >=2.15.

@cashlalala

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2018

@sselberg thanks for your suggestions and reminding, they're really helpful.

  • [NEEDS] Check if WIP/private is supported
  • [NEEDS] Exclude WIP|private changes

I've already updated the pr for 1 & 2.

  • State change to private and or WIP when build is already started
  • State toggling

For 3, 4, IMHO, I think they're too specific for the users. They should have the ability/privilege to control the behaviors for the scenario you mentioned.

  • Somewhat related concerns regarding drafts

For 5, I think it's out of this PR's scope (at least my original purpose), although your concern is definitely right. Maybe it can be another PR in the future.

@sselberg

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

3,4,5 are concerns that I aimed to address had I not been highjacked by other tasks at $DAY_JOB where 5 is definitely out of scope for this PR and 4 is more of a filosophical nature. 3 might be worth addressing but could be done in a separate PR.
Will look at your new changes tomorrow.

P.s.
Just realized that I had the email from my previous employer as primary so I haven't gotten any notifications. That issue is fixed now. :-)

@sselberg

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2018

Sorry for the (once again) late reply. Still trying to get my GitHub notifications to work properly.

@TWestling

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

@cashlalala it seems that the tests are now throwing NullpointerExceptions instead of timing out.
Actually checking if the Gerrit server is version 2.15 before using WIP and private should also be taken care of.
If you can fix those two issues, we should try to merge this as soon as possible.

@cashlalala

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2018

@sselberg @TWestling thx! I’m now in a vacation in USA, going to check them after I get home. About 4~5days. Sorry for late reponse!

@sselberg

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2018

@cashlalala when will you be back?
At $DAY_JOB we really need this merged before 14/10 (totally our own fault).

@cashlalala

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2018

Then it’okay, I’ll arrive home on Oct. 8th.

@cashlalala

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2018

@sselberg @TWestling

testItWithSecurity – com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.job.rest.BuildCompletedRestCommandJobHudsonTest
testTriggerWithLockedDownInstance – com.sonyericsson.hudson.plugins.gerrit.trigger.LockedDownGerritEventTest

Both these tests failed because it returned null when getting plugin instance,

public static PluginImpl getInstance() {
    Jenkins jenkins = Jenkins.getInstance();
    if (jenkins != null) {
            return jenkins.getPlugin(PluginImpl.class); // who returned null
    } else {
            logger.debug("Error, Jenkins could not be found, so no plugin!");
            return null;
    }
}

and I got some exceptions at local when I tried to re-run the test,

Oct 08, 2018 5:45:19 PM hudson.triggers.SafeTimerTask run
SEVERE: Timer task jenkins.metrics.impl.JenkinsMetricProviderImpl$PeriodicWorkImpl@38d9b050 failed
java.lang.AssertionError: jenkins.metrics.api.Metrics is missing its MetricRegistry
at jenkins.metrics.api.Metrics.metricRegistry(Metrics.java:162)
at jenkins.metrics.impl.JenkinsMetricProviderImpl.updateMetrics(JenkinsMetricProviderImpl.java:374)
at jenkins.metrics.impl.JenkinsMetricProviderImpl.access$000(JenkinsMetricProviderImpl.java:74)
at jenkins.metrics.impl.JenkinsMetricProviderImpl$PeriodicWorkImpl.doRun(JenkinsMetricProviderImpl.java:485)
at hudson.triggers.SafeTimerTask.run(SafeTimerTask.java:51)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:748)

and I printed out the plugin list in the mock jenkins server, it certainly lacked many dependent plugins & the gerrit trigger plugin itself. I thought the root cause is initialization failure when loading the plugins, but I had no idea why it failed at loading "metrics".

Somebody give me a hand?

@cashlalala

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2018

Hey, guys,

Any suggestion for the timeout exceptions? I still don't know why, poor me.

@rsandell

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

Oct 08, 2018 7:17:19 PM com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.EventListener gerritEvent
WARNING: Couldn't find a configured trigger for testProject

Might be the culprit?

@cashlalala

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2018

@rsandell thx for the review & suggestion, going to fix them next week.

@cashlalala

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

Oct 08, 2018 7:17:19 PM com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.EventListener gerritEvent
WARNING: Couldn't find a configured trigger for testProject

Might be the culprit?

@rsandell , I had done some tests at local, and now I know where the issue resulted from, which is the permission setting Setup.lockDown(j);. Without this lock, the two timed-out tests could pass. However, I was wondering maybe that's what these two test cases want to point out , maybe bugs really exist in my PR.

But there're two weird things that I can't figure out, and I don't really know how to fix from my PR. I'm going to list below, perhaps you might give me more suggestions?

  • First,

For example, in testTriggerWithLockedDownInstance case,
image

As you can see, I printed out the projects existed in j which is the jenkinsRule, and the project did exist.
Then when It went to another thread which was triggered by the test event, the project can be seen no more.
image

I reviewed my changes line by line, but I didn't see any change which may potentially caused this bug. Maybe I missed something I didn't know, but I really stuck in this....

  • Second

I've tried to bisect the code changes to run the failed test case, and I found something weird as well. When it came to the first commit I made (Update the pom, upgrade the gerrit event lib from 2.12 to 2.13), the error happened at once. I'm really confused with it, because it (the lockDown function) should be nothing to do with gerrit event lib...

@rsandell

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

Yes lockDown is a test utility method that configure Jenkins with security to only provide read permission to authenticated users. So it basically tests that the trigger works as expected when security is configured, which from Jenkins 2.0 and up it always is.

For any job to be readable/discovered in that security context all nessesary threads needs to be run as ACL.SYSTEM which the test thread isn't but the event thread should be for example.
There is a corresponding test, that tests the same thing but without security to verify that nothing else has broken the behavior.

@rsandell

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

I haven't reviewed everything in this PR yet, so I don't know if there is anything I can point out to be obviously causing this :) I just dove in and looked at the test failures to help you get unstuck :)

@rsandell

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

It is a possibility that there is an issue with sonyxperiadev/gerrit-events#79 that was included in 2.13 of gerrit-events

@cashlalala

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

Yea, I think it's the root cause. I excluded the gerrit-events/pull/79 from 2.13 and tested with the lib, it passed as expected.
So, I guess I should check what happened within the gerrit event PR and maybe file another PR for that.

@rsandell

This comment has been minimized.

Copy link
Member

commented Nov 16, 2018

It should most likely work to add a ACL.impersonate(ACL.SYSTEM) in JenkinsAwareGerritHandler#notifyListeners The "Java 6 way".
Though I decided to try the hard approach and upgrade all dependencies to newer plugin pom and Jenkins core version so I could use Java 7's try with resource for a better fit. And now I've been bogged down for three days trying to solve dependency enforcer rules and quirky behaviours in tests :)

@framillien

This comment has been minimized.

Copy link

commented Jan 2, 2019

Hello, we upgraded recently our Gerrit version to 2.16, and this feature is important for us to not try to build private changes.

Can I help in upgrading to newer pom parent and Jenkins core ?

And happy new year !

@cashlalala

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2019

@framillien, that would be great

@framillien

This comment has been minimized.

Copy link

commented Jan 3, 2019

So, go on ! @cashlalala I start from your branch, and do the following here: https://github.com/AllegorithmicSAS/gerrit-trigger-plugin/commits/cashlalala-wip-private

Commit 59daa77 Minimal patch from @rsandell answer to get working unit tests

Commit 04de68b Upgrade Jenkins plugin to last version and Java runtime to 8 + needed dependencies and code changes.

Commit 5cb35bf Upgrade Jenkins core version for auto-closeable and use it + needed dependencies and code changes.

But ... I think we should split this changes in two pull requests:

  • Keep only minimal change here to fix WIP & Private (commit 59daa77)
  • Create a new pull request for Jenkins core update with two others commits.

@rsandell It's what you expect about Jenkins core and plugin pom parent upgrade ? I try to upgrade only needed dependencies, so, a lot of dependencies are still old. What do you think about doing this upgrade in another PR ?

@framillien

This comment has been minimized.

Copy link

commented Jan 4, 2019

New commit, to fix checkstyle issues: c1fb600

@cashlalala

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2019

I removed the dummy checking per @rsandell 's review, and pick 59daa77 frist

will issue new PR if this PR get merged.
04de68b
5cb35bf
c1fb600

Florian Ramillien
@framillien

This comment has been minimized.

Copy link

commented Jan 10, 2019

New PR for Java 8 & Jenkins core upgrade is now here : #389 . We can follow this two tasks independently.

About current checks fail, you can get commit c018dec to fix Checkstyle errors: https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fgerrit-trigger-plugin/detail/PR-383/12/pipeline#log-155 . Hope this help.

@cashlalala

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

@framillien thanks for your work. I had a tough week at work so I couldn't fix the check style errors in time. Just FYI. I merged it. wait for @rsandell to review & merge.

@rsandell

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

#389 and #390 is now merged with the ACL.SYSTEM change. Very sorry for the delay.

@cashlalala

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

@rsandell I fixed the conflict and pass the tests

@rsandell
Copy link
Member

left a comment

Nothing major except some nits and lack of unit tests.

@cashlalala

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

@rsandell I added basic unit tests per the review comment, please have a review. thx~~

@rsandell
Copy link
Member

left a comment

I'll fix the nit in post ;)

public PluginPatchsetCreatedEvent(boolean excludeDrafts,
boolean excludeTrivialRebase,
boolean excludeNoCodeChange) {
boolean excludeNoCodeChange,
boolean excludePrivateState,

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 15, 2019

Member

By keep the original signature with deprecation I meant the original signature before you started making changes :)
We try to strive for keeping binary copatibility between releases of the plugin (since there is no clear way of indicating defined APIs)
And this change breaks binary compatibility because the signature PluginPatchsetCreatedEvent(boolean, boolean) is gone.

This comment has been minimized.

Copy link
@cashlalala

cashlalala Feb 15, 2019

Author Contributor

Sorry for the misunderstanding, I should aware that.
If you still need me to change anything, just inform me.
Thanks for the patience.

@rsandell rsandell merged commit cae900e into jenkinsci:master Feb 15, 2019

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

rsandell added a commit that referenced this pull request Feb 15, 2019

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.