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(admin): identify external plugins #1202

Conversation

ciiay
Copy link
Contributor

@ciiay ciiay commented Apr 23, 2024

Description

As a user of RHDH, I want to know which third party plugins are installed so that I know that there could be unsupported plugins running in my RHDH installation.

Which issue(s) does this PR fix

  • Fixes RHIDP-1854: Identify externally installed plugins in the dynamic-plugins-info table

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

Screenshot(updated 5/1)
rhidp_1854_update_51_1

Note: for internal packages that are not enabled, Version and Role values are left empty in table.
rhidp_1854_update_51_2

Passed e2e tests

➜  e2e-tests git:(rhidp-1854-identify-externally-installed-plugins) ✗ yarn showcase dynamic-plugins-info.spec
yarn run v1.22.19
$ playwright test --project=showcase dynamic-plugins-info.spec

Running 1 test using 1 worker

  ✓  1 … e2e/plugins/dynamic-plugins-info/dynamic-plugins-info.spec.ts:19:3 › dynamic-plugins-info UI tests › it should show a table, and the table should contain techdocs plugins (5.2s)
Dialog message: Failed to sign in as a guest using the auth backend. Do you want to fallback to the legacy guest token?

  1 passed (9.5s)
✨  Done in 10.39s.

Copy link
Contributor

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1202!

1 similar comment
Copy link
Contributor

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1202!

@davidfestal
Copy link
Member

davidfestal commented Apr 25, 2024

I thought we had discussed that this information could, for 1.2, be hard-coded on the client side ?
cc @durandom

I'm of course not against making something more automated, but I'm not a fan of the code depending on the content of the dynamic-plugins.default.yaml to drive the result returned by some new API endpoint.

Another point to consider is the following; the fact that a plugin is mentioned in the dynamic-plugins.default.yaml file does not mean that it is supported, it only means that the plugin is embedded inside the RHDH container image and pre-configured. In other words it tells whether the plugin is external or not.

cc @nickboldt

@ciiay ciiay force-pushed the rhidp-1854-identify-externally-installed-plugins branch from cc7d9cb to a11a300 Compare April 26, 2024 18:47
Copy link
Contributor

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1202!

@ciiay
Copy link
Contributor Author

ciiay commented Apr 26, 2024

Hi @davidfestal , thanks for the review. I also felt that package value in dynamic-plugins.default.yaml isn't very reliable for deciding if a plugin is external or rhdh embedded while working on it. Do we have a list of rhdh embedded plugin I can use as hard coded value for now? I also updated column from Supported to RHDH Embedded, how does it look?

@ciiay ciiay force-pushed the rhidp-1854-identify-externally-installed-plugins branch 3 times, most recently from 2d5c5fd to c416052 Compare April 26, 2024 20:32
Copy link
Contributor

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1202!

@davidfestal
Copy link
Member

cc @gashcrumb as the related EPIC owner.

@ciiay ciiay force-pushed the rhidp-1854-identify-externally-installed-plugins branch from c416052 to 9ad2695 Compare April 29, 2024 21:37
@ciiay ciiay changed the title [WIP]feat(admin): identify external plugins feat(admin): identify external plugins Apr 29, 2024
@ciiay
Copy link
Contributor Author

ciiay commented Apr 29, 2024

Hi @gashcrumb The mapping component seems not passing the SonarCloud duplication check. Did I misunderstand the mapping from our discussion?

Copy link
Contributor

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1202!

@gashcrumb
Copy link
Member

Hi @gashcrumb The mapping component seems not passing the SonarCloud duplication check. Did I misunderstand the mapping from our discussion?

Just took a quick glance (will look closer tomorrow) and I think this is along the lines of what we've discussed. We may have to add an exception in SonarCloud for this (I need to learn how to do this for my own PR too), probably also worth adding a comment if there isn't one already explaining what the mapping is for.

@ciiay ciiay force-pushed the rhidp-1854-identify-externally-installed-plugins branch 2 times, most recently from abaf180 to 643732f Compare April 30, 2024 14:48
Copy link
Contributor

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1202!

@ciiay ciiay force-pushed the rhidp-1854-identify-externally-installed-plugins branch from 643732f to bbe12b4 Compare April 30, 2024 15:30
@gashcrumb
Copy link
Member

@ciiay Probably this line will need updating for the e2e tests. This particular e2e test should be runnable locally without too much hassle, let me know if I can help you set that up.

@gashcrumb
Copy link
Member

Awesome work BTW, swapped this into my existing setup:

image

Copy link
Contributor

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1202!

1 similar comment
Copy link
Contributor

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1202!

@davidfestal
Copy link
Member

Not sure I'm a fan of the Total Plugins wording, even though I don't know what could replace it.

Is there an ability to filter the enabled / disabled ones ? Maybe a Button, something visual that would make it clear what is the list we're seeing ?

@gashcrumb
Copy link
Member

Not sure I'm a fan of the Total Plugins wording, even though I don't know what could replace it.

Maybe just Plugins?

Is there an ability to filter the enabled / disabled ones ? Maybe a Button, something visual that would make it clear what is the list we're seeing ?

The sorting can be used to get an idea but I agree it does feel like a quick filter of some variety would be nicer. I think the filter widget that's used on other catalog pages is probably too much for this purpose. @ShiranHi I wonder if there's other quick filter options we could use here?

@ciiay
Copy link
Contributor Author

ciiay commented Apr 30, 2024

Not sure I'm a fan of the Total Plugins wording, even though I don't know what could replace it.

In the Support pop up, it uses All of the installed plugins. How about Installed plugins or simply Available plugins?

cc @gashcrumb

@ciiay ciiay force-pushed the rhidp-1854-identify-externally-installed-plugins branch from 86a35df to c7156c6 Compare April 30, 2024 17:20
Copy link
Contributor

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1202!

@gashcrumb
Copy link
Member

I think we should probably update the support popup to say "All of the plugins" and I still think maybe just simplifying the page title to just "Plugins" would be best here, as now the table shows all of this. I was thinking this more generic title fits if we also have simple filter control perhaps in the toolbar area for quickly filtering out disabled plugins as @davidfestal suggests.

@gashcrumb
Copy link
Member

There's this filtered table option we could try out.

@ShiranHi
Copy link

ShiranHi commented May 1, 2024

Is there an ability to filter the enabled / disabled ones ? Maybe a Button, something visual that would make it clear what is the list we're seeing ?

The sorting can be used to get an idea but I agree it does feel like a quick filter of some variety would be nicer. I think the filter widget that's used on other catalog pages is probably too much for this purpose. @ShiranHi I wonder if there's other quick filter options we could use here?

I think we don't need to add a filter bar to this page. The columns are sortable, and users can use the search bar if they need to find something specific.

I think we should probably update the support popup to say "All of the plugins" and I still think maybe just simplifying the page title to just "Plugins" would be best here, as now the table shows all of this. I was thinking this more generic title fits if we also have simple filter control perhaps in the toolbar area for quickly filtering out disabled plugins as @davidfestal suggests.

We decided against displaying any links in the UI. This is because the page is hardcoded, meaning any changes would require users to wait for a new release. Additionally, this approach wouldn't function in air-gapped environments. So I don't think we need to add this support button.

I have 3 questions:

  1. I noticed in the table that there's a "Role" column. Could we simply specify whether it's "Backend" or "Frontend"? The current text there seems repetitive. Can we change the column name to be "Type"?
  2. What does it mean if a plugin is pre-installed but not enabled?
  3. Should we include the source, indicating whether this is a Red Hat plugin or a community plugin?

@gashcrumb
Copy link
Member

gashcrumb commented May 1, 2024

Is there an ability to filter the enabled / disabled ones ? Maybe a Button, something visual that would make it clear what is the list we're seeing ?

The sorting can be used to get an idea but I agree it does feel like a quick filter of some variety would be nicer. I think the filter widget that's used on other catalog pages is probably too much for this purpose. @ShiranHi I wonder if there's other quick filter options we could use here?

I think we don't need to add a filter bar to this page. The columns are sortable, and users can use the search bar if they need to find something specific.

I think we should probably update the support popup to say "All of the plugins" and I still think maybe just simplifying the page title to just "Plugins" would be best here, as now the table shows all of this. I was thinking this more generic title fits if we also have simple filter control perhaps in the toolbar area for quickly filtering out disabled plugins as @davidfestal suggests.

We decided against displaying any links in the UI. This is because the page is hardcoded, meaning any changes would require users to wait for a new release. Additionally, this approach wouldn't function in air-gapped environments. So I don't think we need to add this support button.

That's a good point, the support button here is more for the user to configure to point to their own internal pages. This is as good a time as any to remove this control. What about the question of "Installed Plugins" vs "Total Plugins" vs "Plugins" for the table title?

I have 3 questions:

  1. I noticed in the table that there's a "Role" column. Could we simply specify whether it's "Backend" or "Frontend"? The current text there seems repetitive. Can we change the column name to be "Type"?

The "Role" column combines two fields that plugins set in their package.json that comes back from the API. Potentially these could be different values, though it's not well documented how these values are used by the tooling and runtime. But the values as they are do have meaning and relate to how the code works.

  1. What does it mean if a plugin is pre-installed but not enabled?

This means it's a plugin that shipped on the RHDH image but it has not been enabled via the Helm chart or Operator configuration.

  1. Should we include the source, indicating whether this is a Red Hat plugin or a community plugin?

This is what the "Preinstalled" column represents. A "no" here means that an external plugin was installed via a remote souurce. The API doesn't give an installation URL or indication of an external plugin so this is being calculated on the frontend side by comparing the API response against our default configuration.

@gashcrumb
Copy link
Member

BTW we did try the filtered table, however it introduces some layout issues and the actual filter panel doesn't seem to work, which will require additional investigation. I propose we hold off introducing any additional filtering into this PR, we can always do this as an enhancement later.

@ShiranHi
Copy link

ShiranHi commented May 1, 2024

What about the question of "Installed Plugins" vs "Total Plugins" vs "Plugins" for the table title?

Right, I missed that. Only "Plugins" should work imo

@ciiay ciiay force-pushed the rhidp-1854-identify-externally-installed-plugins branch 2 times, most recently from ee37378 to a2d936f Compare May 1, 2024 13:24
Copy link
Contributor

github-actions bot commented May 1, 2024

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1202!

@ciiay ciiay force-pushed the rhidp-1854-identify-externally-installed-plugins branch from 7f39b56 to 2fa1bbd Compare May 1, 2024 14:25
Copy link

sonarcloud bot commented May 1, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
2.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

github-actions bot commented May 1, 2024

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1202!

Copy link
Member

@gashcrumb gashcrumb left a comment

Choose a reason for hiding this comment

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

👍

Copy link

openshift-ci bot commented May 1, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gashcrumb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gashcrumb
Copy link
Member

Huh, didn't realize that would add both labels, a 2nd look by someone else would be good too 😄

@openshift-merge-bot openshift-merge-bot bot merged commit cf3f2c7 into janus-idp:main May 1, 2024
8 checks passed
@gashcrumb
Copy link
Member

Or would have been, I guess it was on the merge queue already even though I removed the label manually. @kadel wonder why both labels were applied at once, was it the 👍 in my comment when I used the approve button?

@ciiay
Copy link
Contributor Author

ciiay commented May 1, 2024

@gashcrumb I can raise a separate PR to address any further comments for this PR.

@davidfestal @ShiranHi Let me know if this PR needs more work. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants