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 configuration of when and how the project list is fetched #220

Merged
merged 3 commits into from Apr 8, 2015

Conversation

Projects
None yet
3 participants
@GLundh
Copy link
Member

commented Mar 30, 2015

Introduces a new configuration section where the admin can configure
how often the project list is queried from Gerrit (used for project
auto completion) and if it should be fetched during startup.

Change-Id: Id4493dc4381d59b9db789e1469c0783bcb2a31f1

Support configuration of when and how the project list is fetched
Introduces a new configuration section where the admin can configure
how often the project list is queried from Gerrit (used for project
auto completion) and if it should be fetched during startup.

Change-Id: Id4493dc4381d59b9db789e1469c0783bcb2a31f1
@GLundh

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2015

Tests work locally. Seems like an unstable test case.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Mar 30, 2015

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

@@ -130,7 +138,7 @@ public void run() {

try {
synchronized (this) {
wait(UPDATE_DELAY);
wait(TimeUnit.SECONDS.toMillis(getConfig().getProjectListRefreshInterval()));

This comment has been minimized.

Copy link
@rsandell

rsandell Mar 31, 2015

Member

This value could be changed during runtime to 0 which should abort it, not cause the thread to never sleep.

How should a change from 0 to >0 be handled? I guess admins are used to clicking that reconnect button?

@@ -230,6 +230,31 @@
checked="${it.config.enableManualTrigger}"
default="${true}"/>
</f:entry>
<f:entry title="${%Fetch Project List Settings}"
help="/plugin/gerrit-trigger/help-GerritFetchProjectList.html">

This comment has been minimized.

Copy link
@rsandell

rsandell Mar 31, 2015

Member

I would prefer we use standard jelly taglib functions as much as possible, since a lot of UI changes are coming. And too much custom html in plugin forms could get broken. Using the taglibs we should get those changes to be compatible "for free".

I think an optionalBlock here could work https://jenkins-ci.org/maven-site/jenkins-core/jelly-taglib-ref.html#form:optionalBlock

This comment has been minimized.

Copy link
@GLundh

GLundh Mar 31, 2015

Author Member

optionalBlock works great when the checkbox should control the visibility of other elements. But in this case the textbox is not dependent on the checkbox. E.g. I would like both to be visible. So I used the divs since I really wanted to align the elements in rows and using a table would not work at all (f:entry + table = broken form submission. I tried everything. It seems to work in heterolists and repeatables, but not directly under f:entries).

So I'm out of ideas. I see tables are used extensively by the Gerrit-Trigger. While it is not optimal, I'm guessing we can make exceptions when the core libs do not want to "play ball"?

This comment has been minimized.

Copy link
@rsandell
@rsandell

This comment has been minimized.

Copy link
Member

commented Mar 31, 2015

@TWestling I see this as a new feature, so holding off the merge until 2.13 strategy is determined.

@rsandell

This comment has been minimized.

Copy link
Member

commented Mar 31, 2015

The build also failed due to There are 1 checkstyle errors. but that could be the fixed in #219

@GLundh

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2015

Thanks for the comments, Bobby :)

Yes, the checkstyle error are not mine. Those appeared after a rebase. So I'm guessing #219 fixes it.

GLundh added some commits Apr 7, 2015

Fixed comments. Allow changing of intervals live.
Change-Id: I336abfac7c9e2ed4d4499a6855c7d1e4371dccc6

@GLundh GLundh force-pushed the GLundh:master branch from f2ac2df to 86c2706 Apr 7, 2015

@GLundh

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2015

@rsandell: Please take a new look.

@@ -130,7 +138,15 @@ public void run() {

try {
synchronized (this) {
wait(UPDATE_DELAY);
long startTime = System.nanoTime();

This comment has been minimized.

Copy link
@rsandell

rsandell Apr 8, 2015

Member

Why nanoTime? Allegedly on certain Windows systems this is a very expensive operation.

This comment has been minimized.

Copy link
@GLundh

GLundh Apr 8, 2015

Author Member

I started with currentTimeMillis, but found several articles that recommends nanoTime instead. It should apparently handle machine time adjustments better. Like someone setting a new date/time on the machine.

rsandell added a commit that referenced this pull request Apr 8, 2015

Merge pull request #220 from GLundh/master
Support configuration of when and how the project list is fetched

@rsandell rsandell merged commit 01214d2 into jenkinsci:master Apr 8, 2015

1 check failed

Jenkins
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.