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.

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

make the seed for YamlSeedConfigPersistence configurable #6409

Merged
merged 3 commits into from
Sep 25, 2021

Conversation

cgardens
Copy link
Contributor

Relates to #6334

What

  • We want the cloud app to use a different seed than OSS. This change makes it so that it possible to customize where YamlSeedConfigPersistence pulls the seed from. My understanding is that changing this class to take in the correct seed should completely solve the issue of the cloud operating on a different index. I will still need to, as a one off, prune out the connectors in cloud that shouldn't be there.
  • @tuliren curious if you think this is a good approach or given that it needs an argument now, I should move to dependency injection.

Recommended reading order

  1. YamlSeedConfigPersistence.java

@github-actions github-actions bot added the area/platform issues related to the platform label Sep 23, 2021
.build();
}

public static void initialize(final Class<?> seedDefinitionsResourceClass) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why you need to do that but this complex object lifecycle has some code smell.

Why is the YamlSeedConfigPersistence a singleton? Are we ever using it without injecting?

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that this class can be constructed differently, I think it should not be a singleton any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool. thanks. this was my intuition too.

@@ -56,6 +56,7 @@

@BeforeAll
public static void dbSetup() {
YamlSeedConfigPersistence.initialize(YamlSeedConfigPersistence.DEFAULT_SEED_DEFINITION_RESOURCE_CLASS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be we have some smiple helper method to construct this class, like YamlSeedConfigPersistence.getDefault(), which automatically use the default resource class? Otherwise people won't remember which class to use.

Copy link
Contributor

@tuliren tuliren Sep 24, 2021

Choose a reason for hiding this comment

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

This initialize method no longer exists. This line can be deleted. Otherwise code cannot compile.

Suggested change
YamlSeedConfigPersistence.initialize(YamlSeedConfigPersistence.DEFAULT_SEED_DEFINITION_RESOURCE_CLASS);

Copy link
Contributor

@tuliren tuliren left a comment

Choose a reason for hiding this comment

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

I think it's better to ditch the singleton pattern and combine the initialize and get method into the following methods:

public YamlSeedConfigPersistence(final Class<?> seedDefinitionsResourceClass) {
  this.allSeedConfigs = ImmutableMap.<SeedType, Map<String, JsonNode>>builder()
    .put(SeedType.STANDARD_SOURCE_DEFINITION, getConfigs(seedDefinitionsResourceClass, SeedType.STANDARD_SOURCE_DEFINITION))
    .put(SeedType.STANDARD_DESTINATION_DEFINITION, getConfigs(seedDefinitionsResourceClass, SeedType.STANDARD_DESTINATION_DEFINITION))
    .build();
}

public static YamlSeedConfigPersistence getDefault() {
  return new YamlSeedConfigPersistence(DEFAULT_SEED_DEFINITION_RESOURCE_CLASS);
}

public static YamlSeedConfigPersistence get(final Class<?> seedDefinitionsResourceClass) {
  return new YamlSeedConfigPersistence(seedDefinitionsResourceClass);
}

Static class variables should preferrably be final.

@@ -100,23 +101,23 @@ public void testUpdateConfigsInNonEmptyDatabase() throws Exception {
@DisplayName("When a connector is in use, its definition should not be updated")
public void testNoUpdateForUsedConnector() throws Exception {
// the seed has a newer version of s3 destination and github source
StandardDestinationDefinition destinationS3V2 = YamlSeedConfigPersistence.get()
final StandardDestinationDefinition destinationS3V2 = YamlSeedConfigPersistence.get()
Copy link
Contributor

Choose a reason for hiding this comment

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

This test shows a good example why the static class variable should not be modified. It is very confusing that we need to call initialize in the setup method first before calling the get here. No one will remember to do this until they get the exception message.

@cgardens cgardens temporarily deployed to more-secrets September 24, 2021 13:04 Inactive
@cgardens
Copy link
Contributor Author

thanks for the review. I agree with both of you. Updated PR to reflect it. @tuliren ready for another review when you are.

@@ -56,6 +56,7 @@

@BeforeAll
public static void dbSetup() {
YamlSeedConfigPersistence.initialize(YamlSeedConfigPersistence.DEFAULT_SEED_DEFINITION_RESOURCE_CLASS);
Copy link
Contributor

@tuliren tuliren Sep 24, 2021

Choose a reason for hiding this comment

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

This initialize method no longer exists. This line can be deleted. Otherwise code cannot compile.

Suggested change
YamlSeedConfigPersistence.initialize(YamlSeedConfigPersistence.DEFAULT_SEED_DEFINITION_RESOURCE_CLASS);

@cgardens cgardens changed the title add initialize to YamlSeedConfigPersistence make the seed for YamlSeedConfigPersistence configurable Sep 25, 2021
@cgardens cgardens merged commit c85becc into master Sep 25, 2021
@cgardens cgardens deleted the cgardens/make_seed_configurable branch September 25, 2021 12:34
@cgardens cgardens temporarily deployed to more-secrets September 25, 2021 12:35 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants