Use commons config and config magic to populate a config object. This in...

[marshallpierce]

...cluded slf4j as a dep, so now we shade in slf4j-simple.

Also switched to using a regex for class matching because that seemed more awesome.

Thoughts especially welcome on config approach, use of properties file vs json, etc. On one hand, it might be nice to have more structure ala json for things like multiple locations for source lookup, but on the other hand YAGNI.

[marshallpierce]

@eirbjo @quidryan thoughts?

[eirbjo]

I'd say we keep this as simple as we can, with as few dependencies as we can. At the same time, we should be open to people and project's different constraints regarding how they run their Java system.

I's say, given a read of property "skybar.port", return the first non-null value of:

• System property (-Dskybar.port=1234)
• Environment variable (SKYBAR_PORT="1234")
• Config file in property format (skybar.port=1234)

As implementation could look something like this:

public class SkybarConfig {
    private final Properties properties;

    public SkybarConfig(Properties properties) {
        this.properties = properties;
    }

    public String get(String propertyName) {
        return firstOf(
                System.getProperty(propertyName),
                System.getenv(propertyName.toUpperCase().replace('.','_')),
                properties.getProperty(propertyName)
        );
    }

    private String firstOf(String... values) {
        for (String value : values) {
            if(value != null) {
                return value;
            }
        }
        return null;
    }
}

The config file could be read from the first non-null of system property -Dskybar.config=skybar.conf or env variable SKYBAR_CONFIG="skybar.conf". If no config file is specified, SkybarConfig can be constructed with an empty Properties, making config file usage optional.

[eirbjo]

YAGNI also for the regex class match pattern. I tend to use regex only as a last resort. It is hard to write correctly, even harder to read. And performance is hard to reason about.

In this case, I think we can do with a combination of prefix includes and excludes, each comma separated lists:

skybar.include=com/skybar, org/springframework skybar.exclude=com/skybar/MessageServlet, org/springframework/core

This will include everything from com/skybar and org/springframework, except the MessageServlet and Spring Framework's "core" package.

I also think the includes and excludes really should be in Java package format:

skybar.include=com.skybar, org.springframework skybar.exclude=com.skybar.MessageServlet, org.springframework.core

Thoughts?

[quidryan]

I concur that that adding that level of configuration seems overblown for a project like this (any other project it would make sense). A hard-coded system of system properties and environment variables will be adequate. That said, I don't understand what you have in mind for sources, so maybe there's requirements for that which I'm not taking into account.

A prefix syntax, of includes and exclude, should be sufficient for any use-case I can think of.

[marshallpierce]

OK, I'll rework this.

For sources I'm imagining something along the lines of "look [here] on the filesystem, look [there] as a maven repo for sources jars".

Unfortunately Java package format is somewhat misleading when it comes to inner classes. com.foo.Bar.Baz turns into com/foo/Bar$Baz so the transformation isn't super clear for what to do with .

[marshallpierce]

I'm leaving in the regex for now; I'll address that in #12.

[marshallpierce]

@eirbjo @quidryan thoughts?

[eirbjo]

Let's keep this PR focused on the SkybarConfig itself. Source strategies and include patterns are interesting topics, but can both be discussed independently of SC. (They are both really just clients of SC)

SkybarConfig looks good to me! For symmetry, I'd like to see SkybarAgent.getSkybarConfig look also for System.env("SKYBAR_CONFIG_FILE") when System.getProperty("skybar.configFile") is null.

And perhaps just use "skybar.config" and "SKYBAR_CONFIG" for brevity?

[marshallpierce]

OK, sounds good; made those changes.

12 is set aside for the include patterns and #4 for source loading.

[marshallpierce]

Any remaining issues before this can be merged?

[eirbjo]

Looks good to me!