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

feat(plugins): Support refactoring out core code #733

Closed

Conversation

robzienert
Copy link
Member

@robzienert robzienert commented Aug 5, 2020

Utility to help refactor core service functionality out into plugins.

An example PluginRefactorRule, for removing kork-pubsub-aws from core could be put in kork-pubsub:

public class AwsPubsubPluginRefactorRule implements PluginRefactorRule {

  private final static String PLUGIN_ID = "io.spinnaker.infra-amazon";
  private final static String IGNORE_KEY = "korkPubsubSqs";

  @Nonnull
  @Override
  public Set<MissingPlugin> checkForMissingPlugins(
    @Nonnull Environment environment,
    @Nonnull SpinnakerPluginManager pluginManager
  ) {
    if (hasAwsPubsubEnabled(environment) && !pluginManager.hasPlugin(PLUGIN_ID)) {
      return Collections.singleton(new MissingPlugin(
        PLUGIN_ID,
        "'amazon.pubsub.enabled' is true, but SQS support has been moved out of core into a plugin.",
        IGNORE_KEY,
        this,
        null
      ));
    }
    return Collections.emptySet();
  }

  private static boolean hasAwsPubsubEnabled(Environment environment) {
    return environment.getProperty("pubsub.amazon.enabled", Boolean.class, false);
  }
}

✏️ I had originally written this with the concept of auto-downloading plugins, but I've rolled that back in favor of hard-crashing the service. Downloading on behalf of the operator can be revisited down the road if need be.

Utility to help refactor core service functionality out into plugins.

An example `PluginRefactorRule`, for removing `kork-pubsub-aws` from core could be put in `kork-pubsub`:

```java
public class AwsPubsubPluginRefactorRule implements PluginRefactorRule {

  private final static String PLUGIN_ID = "io.spinnaker.infra.amazon";
  private final static String IGNORE_KEY = "korkPubsubSqs";

  @nonnull
  @OverRide
  public Set<MissingPlugin> checkForMissingPlugins(
    @nonnull Environment environment,
    @nonnull SpinnakerPluginManager pluginManager
  ) {
    if (hasAwsPubsubEnabled(environment) && !pluginManager.hasPlugin(PLUGIN_ID)) {
      return Collections.singleton(new MissingPlugin(
        PLUGIN_ID,
        "'amazon.pubsub.enabled' is true, but SQS support has been moved out of core into a plugin.",
        IGNORE_KEY,
        this,
        null
      ));
    }
    return Collections.emptySet();
  }

  private static boolean hasAwsPubsubEnabled(Environment environment) {
    return environment.getProperty("pubsub.amazon.enabled", Boolean.class, false);
  }
}
```
@robzienert robzienert requested review from jonsie and a team August 5, 2020 19:38
@@ -20,7 +20,8 @@ dependencies {
implementation("com.squareup.retrofit2:converter-jackson")
implementation("com.squareup.okhttp3:logging-interceptor")

implementation "org.pf4j:pf4j"
// Needs to be api because SpinnakerPluginManager extends DefaultPluginManager.
api "org.pf4j:pf4j"
Copy link
Member Author

Choose a reason for hiding this comment

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

I was kicking around the idea of a PluginService that would encapsulate the PluginManager, allowing this to stay implementation, but decided against out of laziness. I can revisit if we want.


@Bean
public static PluginRefactorRule amazonSqsPluginRefactorRule() {
return new AwsPubsubPluginRefactorRule();
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning on adding this here?

fun toMessage(): String =
"${source.javaClass.simpleName} has detected plugin '${pluginId}'${minVersionMessage()} is missing: ${message.trim('.')}. " +
"If you know this plugin's capabilities are satisfied another way, you may ignore this rule by setting " +
"'${ignoreConfig(ignoreKey)}' to 'true'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Good message.

Copy link
Contributor

@jonsie jonsie 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 you need to either add that pubsub rule or remove it, but either way this looks like a useful idea down the road.

* Helps core services and libraries break existing functionality out into plugins by helping operators detect
* removed functionality.
*/
interface PluginRefactorRule {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add @Alpha here?

* if a service is configured for functionality that was built into the core service offering, but has since
* been refactored out into a plugin.
*/
class PluginRefactorService(
Copy link
Contributor

Choose a reason for hiding this comment

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

And add @Alpha here too I think.

@robzienert
Copy link
Member Author

Will revisit another time.

@robzienert robzienert closed this Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants