bitsofinfo / hazelcast-docker-swarm-discovery-spi

Docker Swarm based discovery strategy SPI for Hazelcast enabled applications
Apache License 2.0
39 stars 33 forks source link

Constructor overload that includes swarmMgrUri #22

Open anton-johansson opened 6 years ago

anton-johansson commented 6 years ago

I find it strange that all properties can be set through a constructor of SwarmMemberAddressProvider except swarmMgrUri (and skipVerifySsl, but that is of less importance). It can only be set through an environment variable. I would like to keep my settings in one place, and I'm currently creating my Hazelcast config programmatically, like this:

        NetworkConfig network = new NetworkConfig();
        network.setPublicAddress(publicAddress);
        network.setPort(port);
        network.setPortAutoIncrement(false);
        network.setJoin(join);

        if (dockerSwarmEnabled)
        {
            SwarmMemberAddressProvider memberAddressProvider = new SwarmMemberAddressProvider(dockerSwarmNetworkName, "", dockerSwarmServiceName, port);

            MemberAddressProviderConfig memberAddressProviderConfig = new MemberAddressProviderConfig();
            memberAddressProviderConfig.setEnabled(true);
            memberAddressProviderConfig.setImplementation(memberAddressProvider);

            network.setMemberAddressProviderConfig(memberAddressProviderConfig);
        }

... and there's no way of setting the swarm manager URI. :(

anton-johansson commented 6 years ago

Actually, it might make more sense to set these specific settings through environment variables... Considering they aren't really application settings, but rather settings related to the Swarm cluster.

I'll think about this for a while, then maybe close the issue.

bitsofinfo commented 6 years ago

having it set via constructor option is not bad either, as long as still works as-is, please submit a PR and will put it in, won't hurt

anton-johansson commented 6 years ago

Yeah, if I need the constructor, I'll gladly submit a PR!

However, as I was thinking about setting them all through environment variables, I noticed that only the swarm manager URI can be set through environment variables. So that plans goes out the window as well. :(

What would you think about allowing setting them all through environment variables? For example, first checking the system properties. If they aren't set, check environment variables. If they aren't set, it will obviously fail.

EDIT: Actually, constructor is good. I can handle the environment variables myself!

bitsofinfo commented 6 years ago

Sure, please submit a PR to make all the configurable probs editable via env vars. Thanks!