[BREAKINGCHANGE] remove dashbaord resource dependecy#97
[BREAKINGCHANGE] remove dashbaord resource dependecy#97
Conversation
facc8a6 to
872354c
Compare
| Such a decision would affect DashbaordProvider and buildDatasourceProxyUrl | ||
| https://github.com/perses/perses/issues/4016 | ||
| */ | ||
| export interface DashboardMinimalResource { |
There was a problem hiding this comment.
If we are moving this out of the spec, why don't we use the same name. It would be a breaking change but for consumers just the import will change.
| export interface DashboardMinimalResource { | |
| export interface DashboardResource { |
There was a problem hiding this comment.
This probably should have its own file.
|
|
||
| if (shape === 'cr-v1alpha1') { | ||
| const name = dashboard.metadata.name.toLowerCase().replace(/[^a-z0-9-]/g, '-'); | ||
| const name = (dashboard.metadata.name as string)?.toLowerCase().replace(/[^a-z0-9-]/g, '-'); |
There was a problem hiding this comment.
We should not cast without prior validation or adding a typeGuard, otherwise we will get runtime errors. Is this cast necessary?
| */ | ||
| export interface DashboardMinimalResource { | ||
| kind: DashboardKind; | ||
| name: string; |
There was a problem hiding this comment.
Why are we adding the name if metadata already has it? Which one would be the source of thruth?
| proxyUrl: buildDatasourceProxyUrl(datasourceApi, { | ||
| project: dashboardResource.metadata.project, | ||
| dashboard: dashboardResource.metadata.name, | ||
| project: dashboardResource.metadata.project as string, |
There was a problem hiding this comment.
we should avoid casting without verification or fix the underlining types
| import { createLinksSlice, LinksSlice } from './links-slice'; | ||
|
|
||
| export type DashboardKind = 'Dashboard' | 'EphemeralDashboard'; | ||
| export type DashboardGenericMetaData = Record<string, string | number | string[] | undefined>; |
There was a problem hiding this comment.
We probably have a better type for this, this is too generic and reduces type safety (casting)
There was a problem hiding this comment.
I picked number and string array for the version and tags fields respectively. If they have no usage here, then the type could be limited to
interface Metadata {
name: string,
project: string
}I myself have some doubts there would be any usage of those fields. WDYT?
dc6f02e to
2f6e05e
Compare
Signed-off-by: Seyed Mahmoud SHAHROKNI <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Seyed Mahmoud SHAHROKNI <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Seyed Mahmoud SHAHROKNI <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Seyed Mahmoud SHAHROKNI <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Seyed Mahmoud SHAHROKNI <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Seyed Mahmoud SHAHROKNI <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Seyed Mahmoud SHAHROKNI <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Seyed Mahmoud SHAHROKNI <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Seyed Mahmoud SHAHROKNI <seyedmahmoud.shahrokni@amadeus.com>
2f6e05e to
5b15e50
Compare
| import { DashboardSpec } from '@perses-dev/spec'; | ||
|
|
||
| export type DashboardKind = 'Dashboard' | 'EphemeralDashboard'; | ||
| export type DashboardMetaData = { name: string; project: string }; |
There was a problem hiding this comment.
Do we have this type as part of the spec? I believe we are using the same metadata for all resources, so define a new one only for this resource might create discrepancies
There was a problem hiding this comment.
We don't have them in the Spec as far as I know. And it seems they should not be part of the Spec as they are irrelevant there. At the moment the the original ones reside in the Core and should not be moved to the Spec. At least this is what I have understood. If we want to eventually drop the Core, we need to take care of the dependency in Shared.
A solution
What I can do is to flatten the DashboardMetaData and keep its filed in the main object (name and project). But, still not sure if this mitigate your concern. For the Kind however, there is nothing I can do. Kind has been used in the Shared, and has been exposed from the Shared. I am not sure what consequence we may encounter, if we drop it. > The main question is if removing the Kind would be a BREAKINGCHANGE which could be resolved.
Description
There is an ongoing effort to remove the core the dependency and replace them with what we have in the Spec.
This PR removes some of the dependencies (mostly DashboardResource, EphemeralDashboardResource) which have been already mentioned by @Nexucis
How does it replace the dependencies?
The PR introduces a new interface which represents an internal definition of any-Dashboard-Resource named DashboardMinimalResource.
Checklist
[<catalog_entry>] <commit message>naming convention using one of thefollowing
catalog_entryvalues:FEATURE,ENHANCEMENT,BUGFIX,BREAKINGCHANGE,DOC,IGNORE.UI Changes
See e2e docs for more details. Common issues include: