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.

Sign up for GitHub

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

Jump to bottom

Fix config diff test #5530

Merged
merged 4 commits into from
Sep 6, 2021
Merged

Fix config diff test #5530

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
85f10a3
Fix KafkaBrokerConfigurationDiffTest
sknot-rh Sep 6, 2021
ca86153
review comments
sknot-rh Sep 6, 2021
cd991cc
reenable
sknot-rh Sep 6, 2021
ab0ec55
review comments
sknot-rh Sep 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 23 additions & 4 deletions .../java/io/strimzi/operator/cluster/operator/resource/KafkaBrokerConfigurationDiffTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

import java.io.IOException;
import java.io.InputStream;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
Expand All @@ -37,6 +39,24 @@ public class KafkaBrokerConfigurationDiffTest {
KafkaVersion kafkaVersion = VERSIONS.version(KAFKA_VERSION);
private int brokerId = 0;



sknot-rh marked this conversation as resolved.
Show resolved Hide resolved
private ConfigEntry instantiateConfigEntry(String name, String val) {
// use reflection to instantiate ConfigEntry
Constructor constructor;
ConfigEntry configEntry = null;
{
try {
constructor = ConfigEntry.class.getDeclaredConstructor(String.class, String.class, ConfigEntry.ConfigSource.class, boolean.class, boolean.class, List.class, ConfigEntry.ConfigType.class, String.class);
constructor.setAccessible(true);
configEntry = (ConfigEntry) constructor.newInstance(name, val, ConfigEntry.ConfigSource.DEFAULT_CONFIG, false, false, emptyList(), ConfigEntry.ConfigType.STRING, "doc");
} catch (NoSuchMethodException | InstantiationException | IllegalAccessException | InvocationTargetException e) {
fail();
}
}
return configEntry;
}

private String getDesiredConfiguration(List<ConfigEntry> additional) {
try (InputStream is = getClass().getClassLoader().getResourceAsStream("desired-kafka-broker.conf")) {
String desiredConfigString = TestUtils.readResource(is);
Expand All @@ -61,8 +81,7 @@ private Config getCurrentConfiguration(List<ConfigEntry> additional) {
configList.forEach(entry -> {
String[] split = entry.split("=");
String val = split.length == 1 ? "" : split[1];
ConfigEntry ce = new ConfigEntry(split[0].replace("\n", ""), val);
entryList.add(ce);
entryList.add(instantiateConfigEntry(split[0].replace("\n", ""), val));
});
for (ConfigEntry ce : additional) {
entryList.add(ce);
Expand Down Expand Up @@ -254,10 +273,10 @@ public void testChangedListenerSecurityProtocolMapFromNonDefault() {

@Test
public void testChangedMoreProperties() {
ArrayList<ConfigEntry> ces = new ArrayList<>();
ArrayList<ConfigEntry> ces = new ArrayList<>(3);
ces.add(new ConfigEntry("inter.broker.listener.name", "david"));
ces.add(new ConfigEntry("group.min.session.timeout.ms", "42"));
ces.add(new ConfigEntry("host.name", "honza"));
ces.add(new ConfigEntry("zookeeper.sync.time.ms", "8000"));
Comment on lines 274 to +276
Copy link
Member

Choose a reason for hiding this comment

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

Can we add comments for why we chose these three configs specifically?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no specific reason. We are doing just 3 random changes. Property host.name was removed from kafka 3.0.0 config model, so we just switched to another property.

Copy link
Member

Choose a reason for hiding this comment

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

Might be still worth a comment that they are randomly chosen without any deeper meaning ... even that is an information which might be useful.

KafkaBrokerConfigurationDiff kcd = new KafkaBrokerConfigurationDiff(Reconciliation.DUMMY_RECONCILIATION, getCurrentConfiguration(emptyList()),
getDesiredConfiguration(ces), kafkaVersion, brokerId);
assertThat(kcd.getDiffSize(), is(3));
Expand Down