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

Scope lut caches data adapters frontend

Open simonychuang opened this issue 2 years ago • 9 comments

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.

simonychuang avatar Jul 27 '22 20:07 simonychuang

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.

image

danotorrey avatar Aug 03 '22 14:08 danotorrey

@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.

kingzacko1 avatar Aug 03 '22 15:08 kingzacko1

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?

image

image

image

@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 avatar Aug 03 '22 16:08 simonychuang

@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 avatar Aug 03 '22 17:08 zeeklop

@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

danotorrey avatar Aug 03 '22 19:08 danotorrey

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.

kingzacko1 avatar Aug 05 '22 19:08 kingzacko1

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.

image

@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 avatar Aug 09 '22 15:08 ryan-carroll-graylog

@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.

zeeklop avatar Aug 09 '22 18:08 zeeklop

@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.

ryan-carroll-graylog avatar Aug 09 '22 18:08 ryan-carroll-graylog

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.

linuspahl avatar Aug 23 '22 15:08 linuspahl

Oh, I just saw this PR is not going into the master branch.

linuspahl avatar Aug 23 '22 15:08 linuspahl