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-48155: gerrit-trigger-plugin does not repsect dynamicTriggerConfiguration flag #336

Merged
merged 1 commit into from Dec 6, 2017

Conversation

Projects
None yet
2 participants
@Jimilian
Copy link
Contributor

commented Dec 4, 2017

If user does not want to use dynamic configuration anymore, let's assign
the list of projects to empty list to avoid possible race conditions.

Also instead of firing update with empty url, better to check it.

@@ -246,13 +245,6 @@ private static String regexEscapted(char symbol) {
public static List<GerritProject> fetch(String gerritTriggerConfigUrl, String serverName)
throws IOException, ParseException {

if (gerritTriggerConfigUrl == null) {

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 4, 2017

Member

I see no reason to remove this, it will just cause another MalformedURLException to be thrown from the URL constructor with a more cryptic message.

This comment has been minimized.

Copy link
@Jimilian

Jimilian Dec 4, 2017

Author Contributor

Because updateTriggerConfigURL was trigged only from once place and now we have a check directly in this place.
Should I keep it to double check?

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 6, 2017

Member

Well it's a public method so it's best to keep it I think :)

@@ -1731,14 +1733,6 @@ public void updateTriggerConfigURL() {
String triggerInformationMessage = MessageFormat.format(
"ParseException when fetching dynamic trigger url: {0}", pe.getMessage());
triggerInformationAction.setErrorMessage(triggerInformationMessage);
} catch (MalformedURLException mue) {
String logErrorMessage = MessageFormat.format(
"MalformedURLException for project: {0} and URL: {1} Message: {2}",

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 4, 2017

Member

Why is this removed? The user could still specify a malformed url in the config screen, and now the error will be harder to find.

This comment has been minimized.

Copy link
@Jimilian

Jimilian Dec 4, 2017

Author Contributor

Agree, will restore.

@@ -1720,8 +1723,7 @@ public void updateTriggerConfigURL() {
triggerInformationAction.setErrorMessage("Dynamic trigger configuration needs "
+ "a specific configured server");
} else {
List<GerritProject> fetchedProjects = GerritDynamicUrlProcessor.fetch(triggerConfigURL, serverName);
dynamicGerritProjects = fetchedProjects;
dynamicGerritProjects = GerritDynamicUrlProcessor.fetch(triggerConfigURL, serverName);

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 4, 2017

Member

Why this change?
If there is an error now you will be stuck with not triggering on anything instead of at least triggering on what was parsed in the last fetch.

This comment has been minimized.

Copy link
@Jimilian

Jimilian Dec 4, 2017

Author Contributor

if fetch will raise a exception - assignment will not happen. Do not see any difference - just use one variable less.

@@ -59,6 +59,9 @@ public void run() {
if (trigger == null) {
return;
}
if (trigger.getTriggerConfigURL() == null || trigger.getTriggerConfigURL().isEmpty()) {

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 4, 2017

Member

use StringUtils.isEmpty()

@Jimilian Jimilian force-pushed the Jimilian:JENKINS_48155 branch from 2eaa4c3 to d2f78e4 Dec 5, 2017

JENKINS-48155: gerrit-trigger-plugin does not repsect dynamicTriggerC…
…onfiguration flag

If user does not want to use dynamic configuration anymore, let's assign
the list of projects to empty list to avoid possible race conditions.

@Jimilian Jimilian force-pushed the Jimilian:JENKINS_48155 branch from d2f78e4 to 756a09e Dec 5, 2017

@Jimilian

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

@rsandell Now it's the most conservative PR ever ;)

@rsandell rsandell merged commit afab9e9 into jenkinsci:master Dec 6, 2017

1 check passed

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

@Jimilian Jimilian deleted the Jimilian:JENKINS_48155 branch Dec 6, 2017

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.