graylog2-server
graylog2-server copied to clipboard
Scope lut caches data adapters frontend
Description
/jenkins-pr-deps Graylog2/graylog-plugin-enterprise#3901
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.
I believe we will need to hide Illuminate-scoped adapter and caches on the Create/Edit lookup table page. These lookup tables are considered to be read-only, and can come/go with Illuminate installations, so I think we will want to prevent the user from creating new caches/adapters that have the Illuminate scope. Pinging @kingzacko1 in case you have more info on this. I can't remember if this is for-sure the decision we reached.
@danotorrey that's correct. The list will need to be filtered of any Illuminate caches/data adapters and we may want to also look at a strictly backend check so that calls directly from the API don't complete successfully if they reference an Illuminate scoped entity.
For Illuminate-scoped entities, it looks like the Actions are showing as disabled/greyed out for adapters, but are not showing for caches and lookup tables. Is this intentional?
@danotorrey I believe they should be consistent, for the data adapters I made the buttons disabled but still showing because we discussed at the beginning of the cycle that simply hiding the buttons may be a confusing user experience. So I think we should do the same for caches and lookup tables too. @zeeklop @mikedklein what do you think?
@simonychuang @danotorrey Regarding hiding or disabling the buttons on the list or any other view, I personally lean towards hiding them (in this case not getting mounted on the view) A disabled button can be enabled using the browser dev tools. A button that isn't there can't be added. This is just a personal preference, I'm good with whatever you guys decided. This isn't critical since the API also is checking for the scope permissions.
@zeeklop @simonychuang I think I had originally suggested disabling the buttons, but, I was checking, and it looks like there might already be a convention in Graylog for hiding buttons (see the Create Stream button below). So, perhaps it might be best to consider hiding the buttons for consistency.
https://github.com/Graylog2/graylog2-server/blob/12e87a3e45727bf14c6b8faef3c9f27b10ad128c/graylog2-web-interface/src/pages/StreamsPage.jsx#L75-L80
Looks like a lot of extraneous commits have been picked up in this PR. Once it gets to a good spot, it may be worthwhile to create a new branch off of Ryan's backend branch and then cherry pick all commits from this feature on top of it.
I believe we will need to hide Illuminate-scoped adapter and caches on the Create/Edit lookup table page. These lookup tables are considered to be read-only, and can come/go with Illuminate installations, so I think we will want to prevent the user from creating new caches/adapters that have the Illuminate scope. Pinging @kingzacko1 in case you have more info on this. I can't remember if this is for-sure the decision we reached.
@simonychuang @zeeklop @danotorrey @kingzacko1 This is looking good! I did notice testing with the latest it looks like Illuminate-scoped Caches and Data Adapters are still selectable when creating/editing a Lookup Table. Is that something we still want to do in this PR?
Also as @kingzacko1 mentioned above is it also worth adding checks in the backend so that saving a Lookup Table that references an Illuminate scoped Cache/Data Adapter fails?
Everything else is looking great! So maybe it's better to get these merged and change the above in other PRs?
@ryan-carroll-graylog
This is looking good! I did notice testing with the latest it looks like Illuminate-scoped Caches and Data Adapters are still selectable when creating/editing a Lookup Table.
Is that something we should change in other PRs?
This PR got really large, lots of components were modified and new test added. I think it would be better to handle this modification in a different PR to also add some tests and so.
@ryan-carroll-graylog
This is looking good! I did notice testing with the latest it looks like Illuminate-scoped Caches and Data Adapters are still selectable when creating/editing a Lookup Table.
Is that something we should change in other PRs?
This PR got really large, lots of components were modified and new test added. I think it would be better to handle this modification in a different PR to also add some tests and so.
Yeah I think that's good plan so we can get these merged.
I just noticed the PR contains the old import path for usePluginEntities
(views/logic/usePluginEntities
), we adjusted this recently https://github.com/Graylog2/graylog2-server/pull/13270
Let's adjust this before we merge this PR.
Oh, I just saw this PR is not going into the master branch.