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

move configuration from ToolInstallation to GlobalConfiguration #43

Merged
merged 25 commits into from
Jan 30, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
371e27b
properly get displayname of node
mwinter69 Dec 11, 2017
67bc346
displayname of node in pipeline
mwinter69 Dec 16, 2017
c252ac8
refactoring
mwinter69 Dec 27, 2017
faf30cd
Merge branch 'master' into configuration
mwinter69 Dec 27, 2017
6b4c6d7
Merge pull request #1 from jenkinsci/master
mwinter69 Dec 27, 2017
333ca41
jenkins and java
mwinter69 Dec 28, 2017
ca4eca8
fix findbugs issues in buildData
mwinter69 Dec 28, 2017
45a03ac
Merge branch 'master' of https://github.com/jenkinsci/logstash-plugin…
mwinter69 Dec 29, 2017
59d1a77
Merge branch 'jenkinsci-master'
mwinter69 Dec 29, 2017
96deff1
Merge pull request #3 from jenkinsci/master
mwinter69 Dec 31, 2017
379c5e4
Merge branch 'master' into configuration
mwinter69 Jan 1, 2018
1321104
javadoc and readme
mwinter69 Jan 1, 2018
4974276
remove unnecessary dependency to workflow-step-api
mwinter69 Jan 1, 2018
227611b
do the comparison if something has changed in the configuration class
mwinter69 Jan 3, 2018
d602a6e
incorporate feedback
mwinter69 Jan 3, 2018
7340194
Merge branch 'configuration2' into configuration
mwinter69 Jan 3, 2018
6116410
adjust link to plugin page
mwinter69 Jan 3, 2018
ac70407
simplify equals
mwinter69 Jan 3, 2018
87c93cb
host based indexers vs uri
mwinter69 Jan 4, 2018
f021a43
add some todos
mwinter69 Jan 4, 2018
fdc79cc
use URL instead of URI
mwinter69 Jan 4, 2018
dac27f1
use URI as persistence for elastic search url
mwinter69 Jan 10, 2018
f2c4a24
fix test
mwinter69 Jan 10, 2018
add8146
use ExpectedException rule
mwinter69 Jan 11, 2018
d28cab4
Merge branch 'master' into configuration
jakub-bochenski Jan 30, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package jenkins.plugins.logstash;

import java.net.URI;
import java.net.URISyntaxException;
import java.util.List;
import java.util.Objects;
import java.util.logging.Level;
import java.util.logging.Logger;

Expand All @@ -10,6 +13,7 @@

import com.cloudbees.syslog.MessageFormat;

import org.apache.http.client.utils.URIBuilder;
import hudson.Extension;
import hudson.init.InitMilestone;
import hudson.init.Initializer;
Expand Down Expand Up @@ -75,6 +79,7 @@ public List<?> getIndexerTypes()
return LogstashIndexer.all();
}

@SuppressWarnings("deprecation")
@Initializer(after = InitMilestone.JOB_LOADED)
public void migrateData()
{
Expand All @@ -97,13 +102,21 @@ public void migrateData()
break;
case ELASTICSEARCH:
LOGGER.log(Level.INFO, "Migrating logstash configuration for Elastic Search");
ElasticSearch es = new ElasticSearch();
es.setHost(descriptor.getHost());
es.setPort(descriptor.getPort());
es.setKey(descriptor.getKey());
es.setUsername(descriptor.getUsername());
es.setPassword(descriptor.getPassword());
logstashIndexer = es;
URI uri;
try
{
uri = (new URIBuilder(descriptor.getHost()))
.setPort(descriptor.getPort())
.setPath("/" + descriptor.getKey()).build();

Choose a reason for hiding this comment

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

not sure if this will work properly for keys that already start with /
does this UriBuilder collapse consecutive /?

Copy link
Author

@mwinter69 mwinter69 Jan 4, 2018

Choose a reason for hiding this comment

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

This is exactly the same behaviour as it was before in the ElasticSearchDao

Choose a reason for hiding this comment

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

OK then

ElasticSearch es = new ElasticSearch(uri.toString());
es.setUsername(descriptor.getUsername());
es.setPassword(descriptor.getPassword());
logstashIndexer = es;
}
catch (URISyntaxException e)
{
LOGGER.log(Level.INFO, "Migrating logstash configuration for Elastic Search failed: " + e.toString());
}
break;
case RABBIT_MQ:
LOGGER.log(Level.INFO, "Migrating logstash configuration for RabbitMQ");
Expand Down Expand Up @@ -151,8 +164,7 @@ public boolean configure(StaplerRequest staplerRequest, JSONObject json) throws
{
// when we bind the stapler request we get a new instance of logstashIndexer
staplerRequest.bindJSON(this, json);

if (logstashIndexer != null && !logstashIndexer.equals(activeIndexer))
if (!Objects.equals(logstashIndexer, activeIndexer))

Choose a reason for hiding this comment

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

can you add further comment explaining that we check for equality because Indexer instance is a holder for Dao

{
activeIndexer = logstashIndexer;
}
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/jenkins/plugins/logstash/LogstashWriter.java
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ public class LogstashWriter {
private boolean connectionBroken;
private Charset charset;

/*
* TODO: the charset must not be transfered to the dao. The dao is shared between different build.
*/
public LogstashWriter(Run<?, ?> run, OutputStream error, TaskListener listener, Charset charset) {
this.errorStream = error != null ? error : System.err;
this.build = run;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
package jenkins.plugins.logstash.configuration;

import java.net.MalformedURLException;
import java.net.URL;
import java.net.URI;
import java.net.URISyntaxException;

import org.apache.commons.lang.StringUtils;
import org.kohsuke.stapler.DataBoundConstructor;
Expand All @@ -16,24 +17,27 @@

public class ElasticSearch extends LogstashIndexer<ElasticSearchDao>
{
protected String key;
protected String username;
protected Secret password;
private String username;
private Secret password;
private URI uri;

@DataBoundConstructor
public ElasticSearch()
public ElasticSearch(String uri) throws URISyntaxException

Choose a reason for hiding this comment

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

Why are we changing the constructor instead of using a @DataBoundSetter?

Copy link
Author

@mwinter69 mwinter69 Jan 4, 2018

Choose a reason for hiding this comment

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

When the uri is not valid it will fail to save the configuration when it is checked in the constructor.
When using a DataBoundSetter, you can save the globalconfiguration with invalid uri

{
this.uri = new URI(uri);
validateUri(this.uri);
}

public String getKey()
public URI getUri()
{
return key;
return uri;
}

@DataBoundSetter
public void setKey(String key)
public void setUri(String value) throws URISyntaxException

Choose a reason for hiding this comment

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

minor; for sake of consistency can we make setter accept URI?

Copy link
Author

Choose a reason for hiding this comment

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

With URI stapler is not able to autoconvert the String to an URI. I will switch to URL as persistence here. Then I can also use a DataBoundSetter that accepts a URL

{
this.key = key;
URI uri = new URI(value);
validateUri(uri);
this.uri = uri;
}

public String getUsername()
Expand Down Expand Up @@ -61,23 +65,23 @@ public void setPassword(String password)
@Override
public boolean equals(Object obj)
{
if (obj == null)
return false;
if (this == obj)
return true;
if (!super.equals(obj))
return false;
if (getClass() != obj.getClass())
return false;
ElasticSearch other = (ElasticSearch) obj;
if (!Secret.toString(password).equals(other.getPassword()))
{
return false;
}
if (key == null)
if (uri == null)
{
if (other.key != null)
if (other.uri != null)
return false;
}
else if (!key.equals(other.key))
else if (!uri.equals(other.uri))
{
return false;
}
Expand All @@ -98,7 +102,7 @@ public int hashCode()
{
final int prime = 31;
int result = super.hashCode();
result = prime * result + ((key == null) ? 0 : key.hashCode());
result = prime * result + ((uri == null) ? 0 : uri.hashCode());
result = prime * result + ((username == null) ? 0 : username.hashCode());
result = prime * result + Secret.toString(password).hashCode();
return result;
Expand All @@ -107,7 +111,7 @@ public int hashCode()
@Override
public ElasticSearchDao createIndexerInstance()
{
return new ElasticSearchDao(host, port, key, username, Secret.toString(password));
return new ElasticSearchDao(uri, username, Secret.toString(password));
}

@Extension
Expand All @@ -125,33 +129,48 @@ public int getDefaultPort()
return 9300;

Choose a reason for hiding this comment

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

umm.. isn't ES default port 9200?

Copy link
Author

Choose a reason for hiding this comment

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

yes but with URI, we don't need this at all.

}

@Override
public FormValidation doCheckHost(@QueryParameter("value") String value)
public FormValidation doCheckUri(@QueryParameter("value") String value)
{
if (StringUtils.isBlank(value))
{
return FormValidation.warning(Messages.PleaseProvideHost());
}
try
{
new URL(value);
URI uri = new URI(value);
validateUri(uri);
}
catch (MalformedURLException e)
catch (URISyntaxException | IllegalArgumentException e)
{
return FormValidation.error(e.getMessage());
}
return FormValidation.ok();
}
}

public FormValidation doCheckKey(@QueryParameter("value") String value)
{
if (StringUtils.isBlank(value))
/**
* Validates that the given uri has a scheme, a port and a path which is not empty or just "/"
*
* @param uri
* @throws IllegalArgumentException when one of scheme, port or path
*/
public static void validateUri(URI uri) throws IllegalArgumentException

Choose a reason for hiding this comment

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

since we ignore those I'd add a check enforcing empty username and password (in authority part)

{
try
{
return FormValidation.error(Messages.ValueIsRequired());
uri.toURL();
}
catch (MalformedURLException e)
{
throw new IllegalArgumentException(e.getMessage());
}

return FormValidation.ok();
if(uri.getPort() == -1) {

Choose a reason for hiding this comment

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

Why not let users rely on default port?
https://logstash.internal/index looks better than https://logstash.internal:443/index

throw new IllegalArgumentException("Please specify a port.");
}

if(StringUtils.isBlank(uri.getPath()) || uri.getPath().trim().matches("^\\/+$")) {

Choose a reason for hiding this comment

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

I have mixed feelings here.
It's obviously useful for 99% of usages, but probably somebody somewhere with some weird Apache/modrewrite in front will want to use an empty path.

Can we make this into a warning instead?

Copy link
Author

Choose a reason for hiding this comment

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

before it was also a must to have a non empty key

throw new IllegalArgumentException("Please specify an elastic search key.");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
package jenkins.plugins.logstash.configuration;

import org.kohsuke.stapler.DataBoundSetter;

import jenkins.plugins.logstash.persistence.AbstractLogstashIndexerDao;

public abstract class HostBasedLogstashIndexer<T extends AbstractLogstashIndexerDao> extends LogstashIndexer<T>
{
private String host;
private int port;

/**
* Returns the host for connecting to the indexer.
*
* @return Host of the indexer
*/
public String getHost()
{
return host;
}

/**
* Sets the host for connecting to the indexer.
*
* @param host
* host to connect to.
*/
@DataBoundSetter
public void setHost(String host)
{
this.host = host;
}

/**
* Returns the port for connecting to the indexer.
*
* @return Port of the indexer
*/
public int getPort()
{
return port;
}

/**
* Sets the port used for connecting to the indexer
*
* @param port
* The port of the indexer
*/
@DataBoundSetter
public void setPort(int port)
{
this.port = port;
}

@Override
public int hashCode() {
final int prime = 31;
int result = 1;
result = prime * result + ((host == null) ? 0 : host.hashCode());
result = prime * result + port;
return result;
}

@Override
public boolean equals(Object obj) {
if (this == obj)
return true;
if (obj == null)
return false;
if (getClass() != obj.getClass())
return false;
HostBasedLogstashIndexer<?> other = (HostBasedLogstashIndexer<?>) obj;
if (host == null) {
if (other.host != null)
return false;
} else if (!host.equals(other.host))
return false;
if (port != other.port)
return false;
return true;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package jenkins.plugins.logstash.configuration;

import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;

public class HostConfiguration
Copy link

@jakub-bochenski jakub-bochenski Jan 4, 2018

Choose a reason for hiding this comment

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

I can't find any references to that class, am I missing something?

{

protected String host;
protected int port;

@DataBoundConstructor
public HostConfiguration()
{
}

public String getHost()
{
return host;
}

@DataBoundSetter
public void setHost(String host)
{
this.host = host;
}

public int getPort()
{
return port;
}

@DataBoundSetter
public void setPort(int port)
{
this.port = port;
}

@Override
public int hashCode() {
final int prime = 31;
int result = 1;
result = prime * result + ((host == null) ? 0 : host.hashCode());
result = prime * result + port;
return result;
}

@Override
public boolean equals(Object obj) {
if (this == obj)
return true;
if (obj == null)
return false;
if (getClass() != obj.getClass())
return false;
HostConfiguration other = (HostConfiguration) obj;
if (host == null) {
if (other.host != null)
return false;
} else if (!host.equals(other.host))
return false;
if (port != other.port)
return false;
return true;
}
}
Loading