graylog2-server icon indicating copy to clipboard operation
graylog2-server copied to clipboard

Immutable Lookup Tables, Caches, and Data Adapters backend

Open ryan-carroll-graylog opened this issue 2 years ago • 4 comments

Add immutable entities support for Lookup Tables, Caches, and Data Adapters.

/jenkins-pr-deps Graylog2/graylog-plugin-enterprise#3901

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Refactoring (non-breaking change)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [ ] My code follows the code style of this project.
  • [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [ ] I have read the CONTRIBUTING document.
  • [ ] I have added tests to cover my changes.

ryan-carroll-graylog avatar Jul 15 '22 12:07 ryan-carroll-graylog

Looks good to me.

Thanks for all the feedback.

ryan-carroll-graylog avatar Jul 20 '22 16:07 ryan-carroll-graylog

@ryan-carroll-graylog This LGTM and tests successfully! There is only one small method override that needs to be added in order for the Illuminate installation to automatically inject the scope:

This method override will need to added to all of the facades, so that the injector below will know which entity facades the ILLUMINATE scope can be injected for. Without this, the DEFAULT scope is used for Illuminate-installed content packs.

@Override
    public boolean usesScopedEntities() {
        return true;
    }

https://github.com/Graylog2/graylog-plugin-enterprise/blob/923d2d3dbdfde70d7bffd987ea32a1cb84ff7afb/enterprise/src/main/java/org/graylog/plugins/illuminate/installation/ContentPackScopeInjector.java#L46-L50

@danotorrey I was just looking at this. Do these LUT entities need this even though they'll be installed outside the Illuminate content-pack pipeline, in the new code for @kingzacko1 changes?

I suppose it's probably good form to have it anyway for consistency in case we decide to use this elsewhere.

ryan-carroll-graylog avatar Jul 28 '22 20:07 ryan-carroll-graylog

@ryan-carroll-graylog This LGTM and tests successfully! There is only one small method override that needs to be added in order for the Illuminate installation to automatically inject the scope: This method override will need to added to all of the facades, so that the injector below will know which entity facades the ILLUMINATE scope can be injected for. Without this, the DEFAULT scope is used for Illuminate-installed content packs.

@Override
    public boolean usesScopedEntities() {
        return true;
    }

https://github.com/Graylog2/graylog-plugin-enterprise/blob/923d2d3dbdfde70d7bffd987ea32a1cb84ff7afb/enterprise/src/main/java/org/graylog/plugins/illuminate/installation/ContentPackScopeInjector.java#L46-L50

@danotorrey I was just looking at this. Do these LUT entities need this even though they'll be installed outside the Illuminate content-pack pipeline, in the new code for @kingzacko1 changes?

I suppose it's probably good form to have it anyway for consistency in case we decide to use this elsewhere.

Just added the overrides of usesScopedEntities() in the facades.

ryan-carroll-graylog avatar Jul 29 '22 17:07 ryan-carroll-graylog

I was just looking at this. Do these LUT entities need this even though they'll be installed outside the Illuminate content-pack pipeline, in the new code for @kingzacko1 changes?

@ryan-carroll-graylog Thanks @ryan-carroll-graylog Good point, sorry, I had gotten confused about this for a bit there. You are correct about that. They are not strictly needed. I agree though, adding them anyhow just in case sounds good to me.

danotorrey avatar Jul 29 '22 20:07 danotorrey