CLI-361: Fixing rest extension bug

[Sanjana Kaundinya (skaundinya15)]

What

Previously, the monitoring rest extension for Replicator introduced a bug that would override any other rest extensions in the worker property file. This PR introduces a check to see if there are existing rest extensions, to append at the end of those rest extensions (instead of overriding it)

References

https://confluentinc.atlassian.net/browse/CLI-361

Test&Review

Tested following cases locally:

1. Works on properties files without rest.extensions.classes config
2. Works on properties files configured with rest.extensions.classes in single line format (with the replicator value)
3. Works on properties files configured with rest.extensions.classes in multi line format (with the replicator value)
4. Works on properties files configured with rest.extensions.classes in multi line format (without the replicator value)
5. Works on properties files configured (but without value) with rest.extensions.classes in single line format (without the replicator value)
6. Logs error if rest.extensions.classes is configured more than once
7. Correctly adds io.confluent.connect.replicator.monitoring.ReplicatorMonitoringExtension even if foo.io.confluent.connect.replicator.monitoring.ReplicatorMonitoringExtension is present or io.confluent.connect.replicator.monitoring.ReplicatorMonitoringExtension.foo is present in either single line or multi line config.

[Chris Egerton (C0urante)]

Thanks for the fix, Sanjana Kaundinya (@skaundinya15)! I think there are a few cases that aren't completely covered with the current changes, though.

[David Hyde (DABH)]

+1 once all PR comments are addressed and we have confirmation that at least manual testing indicates this PR fixes the issue. Thanks!

[Chris Egerton (C0urante)]

This is looking really close and believe me, I'd like to see it merged as much as you guys would. I left another comment on a bug in the current implementation and have a higher-level question I'd like to ask--is there any chance that the user will already have the io.confluent.connect.replicator.monitoring.ReplicatorMonitoringExtension REST extension in their config file? If so, we should catch and handle that case too (by not making any alterations to the config file).

[Sanjana Kaundinya (skaundinya15)]

This is looking really close and believe me, I'd like to see it merged as much as you guys would. I left another comment on a bug in the current implementation and have a higher-level question I'd like to ask--is there any chance that the user will already have the io.confluent.connect.replicator.monitoring.ReplicatorMonitoringExtension REST extension in their config file? If so, we should catch and handle that case too (by not making any alterations to the config file).

That is a good point. To be honest I'm not entirely sure if there is a chance the user would have already configured on there, since this is the local quickstart that gets run from the confluent quickstart website. That being said, for good measure we should catch that case. I'll add that in now along with the other bug you pointed out.

[Sanjana Kaundinya (skaundinya15)]

I ended up refactoring the code out into functions because it was getting annoying to add more long winded code. This one should be a lot more readable now with the separate functions, and also provides usability for injecting other connect configs into properties files in the future.

[Chris Egerton (C0urante)]

Close!

[Sanjana Kaundinya (skaundinya15)]

Chris Egerton (@C0urante) I updated the PR to add support for multi-line configs in the case of checking if the config already exists in the properties file. Ready for another round of review whenever you are.

[Chris Egerton (C0urante)]

LGTM, couple of small comments but feel free to address/not address at your discretion.

Before merging, as a sanity check can we get confirmation that this works locally as expected on properties files that don't contain the rest.extension.classes config, that contain multiple declarations for the config (which should just log an error), ones that do contain it but without the Replicator REST extension configured, and ones that already have the extension configured (and with single- and multi-line values for the last two cases)?

David Hyde (@DABH) this has evolved a bit since your approval, do you want to give it one last look?

[Sanjana Kaundinya (skaundinya15)]

LGTM, couple of small comments but feel free to address/not address at your discretion.

Before merging, as a sanity check can we get confirmation that this works locally as expected on properties files that don't contain the rest.extension.classes config, that contain multiple declarations for the config (which should just log an error), ones that do contain it but without the Replicator REST extension configured, and ones that already have the extension configured (and with single- and multi-line values for the last two cases)?

David Hyde (@DABH) this has evolved a bit since your approval, do you want to give it one last look?

Chris Egerton (@C0urante) Thanks for the thorough review! I just addressed the last two comments and pushed. I have sanity tested the following cases locally as you mentioned:

1. Works on properties files without rest.extensions.classes config
2. Works on properties files configured with rest.extensions.classes in single line format (with the replicator value)
3. Works on properties files configured with rest.extensions.classes in multi line format (with the replicator value)
4. Works on properties files configured with rest.extensions.classes in multi line format (without the replicator value)
5. Works on properties files configured (but without value) with rest.extensions.classes in single line format (without the replicator value)
6. Logs error if rest.extensions.classes is configured more than once
7. Correctly adds io.confluent.connect.replicator.monitoring.ReplicatorMonitoringExtension even if foo.io.confluent.connect.replicator.monitoring.ReplicatorMonitoringExtension is present or io.confluent.connect.replicator.monitoring.ReplicatorMonitoringExtension.foo is present in either single line or multi line config.

David Hyde (@DABH) Would appreciate your final review before I merge in, thanks everyone!

[David Hyde (DABH)]

I am okay with this. There are still limitations / corner cases not handled, as Chris Egerton (@C0urante) has pointed out, but this will work for most customers in the short-term, and in the medium-/long-term we'd like to replace all this bash code with Go code anyway, so I think it's best to merge this now and then try to find resourcing to do the bash->go conversion. Thanks for your work on this