Add support for multiple configuration contexts #48

helpermethod · 2021-08-18T10:43:13Z

here comes the PR! I hope I've got the the requirements right 😄.

One thing I would like to challenge is supporting the old, properties-like config format. Because we are still pre 1.0.0, I guess it would be acceptable to not support the current (properties-like) configuration format anymore. Wdyt?

The current, context-aware format is very flexible, except for the context-name and cluster URL, every other property is
optional.

So both

{
    "currentContext": "local",
    "local": {
        "cluster": "http://localhost:8083"
    }
}

and

{
    "currentContext": "local",
    "local": {
        "cluster": "http://localhost:8083",
        "boostrapServers": "localhost:9092",
        "offsetTopic": "connect-offsets"
    }
}

are valid.

Fixes #24.

gunnarmorling

Nice, this LGTM overall. A few comments inline. Thx, @helpermethod!

pom.xml

gunnarmorling · 2021-08-19T06:20:01Z

src/main/java/dev/morling/kccli/command/SetContextCommand.java


-    @Option(names = { "--cluster" }, description = "URL of the Kafka Connect cluster to connect to", required = true)
+    @Option(names = {"--cluster"}, description = "URL of the Kafka Connect cluster to connect to", required = true)


Should we default the options? http://localhost:8083 and localhost:9092 seem reasonable for KC and broker URLs? For the offset topic, we could leave that optional. If if it's not set for a given context, the future commands interacting with offsets wouldn't work without it, stating the need to specify a value in the chosen context. WDYT?

gunnarmorling · 2021-08-19T06:24:48Z

src/main/java/dev/morling/kccli/util/ConfigurationContext.java

+                configuration.addConfigurationContext(contextName, context)
+            );
+        } catch (IOException e) {
+            throw new RuntimeException("Couldn't write context file ~/" + CONFIG_FILE, e);
        }
    }

    public URI getCluster() {


Should this method go away entirely, and all access be via getContext()? And we'd log an error if no context has been defined.

By logging you mean logging to STDOUT?

Yes, exactly.

gunnarmorling · 2021-08-19T06:26:46Z

src/test/java/dev/morling/kccli/util/ConfigurationContextTest.java

+        @MethodSource("dev.morling.kccli.util.ConfigurationContextTest#setConfigurationArguments")
+        @ParameterizedTest
+        void should_create_a_new_configuration_when_no_configuration_exists(String contextName, Context context, String expectedConfiguration) throws IOException {
+            System.setProperty("user.home", tempDir.getAbsolutePath());


Neat trick :) Alternatively, there could be a constructor in ConfigurationContext which takes the config directory, while the default constructor would use the home dir.

Had the exact same idea, but didn't want to change too much code at first. Good suggestion!

gunnarmorling · 2021-08-19T06:29:52Z

src/main/java/dev/morling/kccli/util/ConfigurationContext.java

+            var configuration = objectMapper.readValue(configPath, Configuration.class);
+
+            return configuration.configurationContexts().get(configuration.getCurrentContext());
+        } catch (IOException e) {
            throw new RuntimeException("Couldn't read configuration file ~/" + CONFIG_FILE, e);


Let's add a sentence like "Create/update your configuration by running 'kcctl set-context ...'".

src/main/java/dev/morling/kccli/service/Context.java

gunnarmorling · 2021-08-19T06:35:52Z

One thing I would like to challenge is supporting the old, properties-like config format. Because we are still pre 1.0.0, I guess it would be acceptable to not support the current (properties-like) configuration format anymore. Wdyt?

Yes, that's ok. We only should log a meaningful error in that case.

Other than that, could you

Rebase this PR so to address the current conflicts
Squash the commits into one and prefix the message with a suitable emoj :)
Add a section to the README describing the notion of contexts; two or three sentences will surely suffice, just to explain the overall approach

Thanks again!

helpermethod · 2021-08-20T19:43:38Z

Now adding a few lines to the README.md.

helpermethod · 2021-08-20T20:12:28Z

Hi @gunnarmorling, that push should resolve most, if not all of your comments 😄.

gunnarmorling · 2021-08-20T20:52:25Z

@helpermethod, could you also re-generate the completion script (see instructions in the README) for the new options?

gunnarmorling · 2021-08-20T21:02:28Z

Thanks a lot, @helpermethod! So I've played around a bit, it looks great overall. I wonder though if we can make the experience a bit smoother by default, i.e. if the user has not configured any context yet. How about the following if no context has been configured:

When invoking any command other than config and its sub-commands, use http://localhost:8083 (as before), and display a warning such as "No configuration context has been defined, using http://localhost:8083 by default. Run ... to create a context."
When invoking config get-context, print a message like "No configuration context has been defined, http://localhost:8083 will be used by default. Run ... to configure a create."

WDYT?

helpermethod · 2021-08-20T21:23:14Z

Great suggestions, I'll incorporate them over the weekend!

gunnarmorling · 2021-08-24T19:04:31Z

@helpermethod, if you'd like me to wrap up this one, just let me know. Would love to have it in, so we can get to the offset reset stuff :)

helpermethod · 2021-08-24T20:01:38Z

Hi @gunnarmorling,

I'm on it, I'm currently stuffed with work so it's not easy to find some uninterrupted time 😄.

I don't quite get the difference between

When invoking any command other than config and its sub-commands, use http://localhost:8083 (as before), and display a warning such as "No configuration context has been defined, using http://localhost:8083 by default. Run ... to create a context."

When invoking config get-context, print a message like "No configuration context has been defined, http://localhost:8083 will be used by default. Run ... to configure a create."

So in any case basically a warning is printed and a default is returned?

gunnarmorling · 2021-08-24T20:20:09Z

So in any case basically a warning is printed and a default is returned?

Yes, that'd be the idea. Only the wording of the messages might be slightly different ("Using..." for actual commands vs. "Will use..." for get-context), but perhaps I'm overthinking it and we can have one which fits for both :)

gunnarmorling · 2021-08-24T20:22:58Z

@helpermethod, there's a few odd changes part of this now, like version downgrades in the POM and removal of the Dependabot config.

helpermethod · 2021-08-24T20:30:23Z

Messed up the latest commit, fixing it right now.

helpermethod · 2021-08-24T20:39:00Z

I hope that should do it 😅!

gunnarmorling · 2021-08-24T20:43:12Z

Yepp, LGTM. Applied. Thanks a lot!

gunnarmorling reviewed Aug 19, 2021

View reviewed changes

gunnarmorling mentioned this pull request Aug 19, 2021

Allow to switch between contexts #19

Closed

helpermethod force-pushed the issue-24 branch 2 times, most recently from c02d392 to b5c24e5 Compare August 20, 2021 19:42

helpermethod force-pushed the issue-24 branch from b5c24e5 to 2c110e1 Compare August 20, 2021 20:11

helpermethod force-pushed the issue-24 branch 2 times, most recently from 8bbbecc to 7c4ac6e Compare August 24, 2021 20:15

helpermethod force-pushed the issue-24 branch from 7c4ac6e to 3dc46f6 Compare August 24, 2021 20:29

helpermethod force-pushed the issue-24 branch from 3dc46f6 to c9e11bc Compare August 24, 2021 20:35

🔧 Add support for multiple configuration contexts

3e6135e

helpermethod force-pushed the issue-24 branch from c9e11bc to 3e6135e Compare August 24, 2021 20:37

gunnarmorling merged commit acfc54c into kcctl:main Aug 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple configuration contexts #48

Add support for multiple configuration contexts #48

helpermethod commented Aug 18, 2021 •

edited

gunnarmorling left a comment

gunnarmorling Aug 19, 2021

helpermethod Aug 20, 2021

gunnarmorling Aug 19, 2021

helpermethod Aug 20, 2021

gunnarmorling Aug 20, 2021

gunnarmorling Aug 19, 2021

helpermethod Aug 20, 2021

helpermethod Aug 20, 2021

gunnarmorling Aug 19, 2021

helpermethod Aug 20, 2021

gunnarmorling commented Aug 19, 2021

helpermethod commented Aug 20, 2021

helpermethod commented Aug 20, 2021

gunnarmorling commented Aug 20, 2021

gunnarmorling commented Aug 20, 2021

helpermethod commented Aug 20, 2021

gunnarmorling commented Aug 24, 2021

helpermethod commented Aug 24, 2021

gunnarmorling commented Aug 24, 2021

gunnarmorling commented Aug 24, 2021

helpermethod commented Aug 24, 2021

helpermethod commented Aug 24, 2021

gunnarmorling commented Aug 24, 2021


		@Option(names = { "--cluster" }, description = "URL of the Kafka Connect cluster to connect to", required = true)
		@Option(names = {"--cluster"}, description = "URL of the Kafka Connect cluster to connect to", required = true)

Add support for multiple configuration contexts #48

Add support for multiple configuration contexts #48

Conversation

helpermethod commented Aug 18, 2021 • edited

gunnarmorling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunnarmorling commented Aug 19, 2021

helpermethod commented Aug 20, 2021

helpermethod commented Aug 20, 2021

gunnarmorling commented Aug 20, 2021

gunnarmorling commented Aug 20, 2021

helpermethod commented Aug 20, 2021

gunnarmorling commented Aug 24, 2021

helpermethod commented Aug 24, 2021

gunnarmorling commented Aug 24, 2021

gunnarmorling commented Aug 24, 2021

helpermethod commented Aug 24, 2021

helpermethod commented Aug 24, 2021

gunnarmorling commented Aug 24, 2021

helpermethod commented Aug 18, 2021 •

edited