Widgets for itless environment #716
Conversation
The FR env has a limited set of features when compared to the commercial environment. Because of this, we should be limiting those within code so that we only return the useful widgets in itless environments. This commit adds a WidgetMappingFR type that specifies a subset of the commercial widgets. Signed-off-by: Stephen Adams <[email protected]>
If we're running in an ITless environment, we need to use the route that hands us the FR default widgets instead of the commercial ones. Here we're using an unleash flag to do that. Signed-off-by: Stephen Adams <[email protected]>
|
@SteveHNH when do you need this? We will soon be using actual CRDs to specify widgets and the widgets should be defined by the available frontends. Just to avoid adding "technical debt" to the repository if we can. |
|
@Hyperkid123 oh that sounds great. I didn't know that was on the roadmap. I have tickets for getting widgets in the itless environment so I'm just trying to get those done. The main part of it is that the we need to make sure we only allow widgets that are useful in that environment, so as long as the CRD approach will allow us to do that, I think that should be fine. Any ETA on that feature? |
|
ETA is Q4. But considering there will also be some transition and onboarding from the app side, its like that thing swill be ready in Q1. So I think you should not wait. |
|
Okay, that's good to know. I appreciate your willingness to help us get this working in the FR environment, but I'm bummed it adds technical debt. I'll continue working with @florkbr on it and try to make it as clean as possible so it's easy to remove when the time comes. |
It turns out we have a feature flag for itless envs already, so we can leverage it rather than creating a new one. Signed-off-by: Stephen Adams <[email protected]>
SteveHNH
changed the title
|
Oh, this was a technical debt from the start. We knew we would replace it eventually. But the changes we are working are not trivial. Don't worry about it. |
rest/routes/dashboardTemplate.go
Outdated
| @@ -280,6 +281,15 @@ func GetWidgetMappings(w http.ResponseWriter, r *http.Request) { | |||
| handleDashboardResponse[models.WidgetModuleFederationMapping](resp, err, w) | |||
| } | |||
|
|
|||
| func GetWidgetMappingsFR(w http.ResponseWriter, r *http.Request) { | |||
There was a problem hiding this comment.
@SteveHNH I wanted to list out a few long term considerations we will need in the future (I know you and I have discussed several of these offline).
- We currently only support one template type (currently "landingPage"). See: https://github.com/RedHatInsights/chrome-service-backend/blob/main/docs/dashboard-layouts.md#base-layouts (backend) and https://github.com/RedHatInsights/widget-layout/blob/master/src/api/dashboard-templates.ts#L13 (frontend).
- We plan to migrate this backend logic to its own service (outside of chrome service) and allow for additional "types" in the future. For example if another UI - such as the insights UI - would want to utilize our common widgetized layout service but render a different template and widgets than our landing page.
- As @Hyperkid123 pointed out - we also will be migrating the individual widget configurations into the applications that surface them (via frontend config and the frontend operator).
- Finally - we need to consider the ordering of widgets by default (when a user first loads the template type and it is automatically forked). I do not believe it is a requirement today - but would we ever want to have a different base layout for the landing page in itless than we do in commercial?
Getting back to this PR though - we have a few options to unblock us in the short term:
- Fork the
widget-mappingitself for itless (this PR). This would pose minimal risk to our commercial flow - however will have duplication between commercial and itless widget-mappings (for those widgets that appear in both lists). We would also need to remember to add any new widgets that itless would be interested in to both lists. - Add a new metadata/config field inside of each widget struct to indicate if it should appear in the itless environment (more self-service model). This would prevent the duplicated widget-mapping (as we would just filter for that metadata field when running inside of itless). However this model doesn't scale well when it comes to adding support for multiple envs, layouts, etc. and could result in widgets appearing in itless unexpectedly if the field is copy/pasted. This also defeats the purpose of having the widget-mapping (whitelist of widgets that can be rendered).
That being said - I think I prefer option 1 (as you have here). My only concern is we are not updating the base layout for itless as well. See https://github.com/RedHatInsights/chrome-service-backend/blob/main/rest/service/dashboardTemplate.go#L450-L507. This means each time the "landingPage" layout if forked when the user first loads the page in itless - we will have references in the template to widgets that do not exist in the itless widget mapping (such as "ansible"). I believe these widgets will simply be ignored by the UI as they do not appear in the mapping - however we should confirm that in our itless environment and confirm customizing the layout works as expected (even with these invalid references).
There was a problem hiding this comment.
@florkbr I'd like to provide a 3rd option. Which is using feature flags to enable/disable items in an environment. rather than adding a flag saying this is for itless. Why?
Well, we want feature flags anyway, it will be easier to maintain without introducing tech debt, and it will have the same effect of "messed up initial layout" anyway.
Chrome service is hooked up to unleash already. We can do some "inverted" feature flags like "widget.supportCase.hidden". This way we can remove the widget from a layout when it gets "forked" or when the struct is created from the base layout yaml
There was a problem hiding this comment.
@Hyperkid123 that seems like a reasonable approach to me - I think I was deep in the itless headspace with the other 2 options - I like that solution more. @SteveHNH thoughts on using @Hyperkid123's suggestion?
There was a problem hiding this comment.
Sounds good to me! I understand the idea, but I don't fully understand how to make that happen. This is also the first time I've messed with this code, so I just need to dive in and get a handle on it. Thanks for the feedback!
- Added a string to each widget to identify its corresponding featureflag - If the 'chrome-service.filterWidgets.enable' flag is set, then a function filters through the widgets to see which ones are meant to be hidden. By using this flag, we prevent searching for disabled widgets when we don't need to. It will simply load the defaults. Signed-off-by: Stephen Adams <[email protected]>
|
I reworked the widget filtering. The only thing that bothers me is the looping through widgets to see which ones to turn on/off. There might be a smarter way to go after this. I feel like this could introduce a little lag by needing to reach out to unleash for each one. The other downside I can see is that if for some reason Unleash is unavailable when a user hits the service, the I'm open to ideas on how we might resolve that if there are any. Maybe invert that filterWidgets check? The worst case would be a commercial customer would see a limited set of widgets until Unleash came back. |
rest/routes/dashboardTemplate.go
Outdated
| filteredWidgets := make(map[models.AvailableWidgets]models.ModuleFederationMetadata) | ||
|
|
||
| for key, value := range widgetMapping { | ||
| if !featureflags.IsEnabled(value.FeatureFlag) { |
There was a problem hiding this comment.
Care to add a comment here, please? Because this is inverted it might be a headscratcher for us when we look at the code in a couple fo weeks. I had to read it twice already 😆
This change modifies the logic for widgets so that instead of creating a new mapping and returning it, we just delete the widget from the original map. This makes it so that we're not using inverse logic and creating confusion where we don't need it. Signed-off-by: Stephen Adams <[email protected]>
|
@Hyperkid123 I went ahead and figured out how to un-invert that logic so that it reads a little more clearly. Added a comment as well. |
|
/retest |
Within the itless environment, not all services are available so not all widgets should be available by default for the customers in that environment. This is meant to resolve that using a feature flag that will be checked to ensure that if a customer is in an itless env, they'll get only a subset of the available widgets.
Working on this a bit with @florkbr.