-
Notifications
You must be signed in to change notification settings - Fork 18
Add version-aware settings support #407
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
base: main
Are you sure you want to change the base?
Add version-aware settings support #407
Conversation
ad46231 to
f14b57d
Compare
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
f160e16 to
2580790
Compare
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
afde4d3 to
ca58a52
Compare
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
common/constants.ts
Outdated
| export const WLM_CONFIG: Readonly<ConfigSchema['wlm']> = Object.freeze({ ...DEFAULT_CONFIG.wlm }); | ||
|
|
||
| // Version constants | ||
| export const VERSION_3_1 = { major: 3, minor: 1 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make them more type safe, like
interface Version {
major: number;
minor: number;
patch?: number;
}
export const VERSIONS = {
2_19: { major: 2, minor: 19 } as Version,
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed because logic change in version-ulits.ts
public/service.ts
Outdated
|
|
||
| export const [getCore, setCore] = createGetterSetter<CoreStart>('Core'); | ||
|
|
||
| export const [getRouteService, setRouteService] = createGetterSetter<RouteService>(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why ''? This could make debugging difficult based on https://github.com/opensearch-project/OpenSearch-Dashboards/blob/e5af133ac3b2f210f115a5cf6f74c88bb9bf545b/src/plugins/opensearch_dashboards_utils/common/create_getter_setter.ts#L38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
public/utils/version-utils.ts
Outdated
| return cachedVersion; | ||
| }; | ||
|
|
||
| const compareVersion = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse semver? that should be the standard osd library to check the versions. I understand you need to consider the snapshot case, you can still build on top of that library instead of rewriting this whole library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reused semver
| } catch (error) { | ||
| console.error('Failed to fetch data source version:', error); | ||
| if (!isComponentUnmounted) { | ||
| setShowLiveQueries(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's error, should the default value be true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Signed-off-by: Kishore Kumaar Natarajan <[email protected]>
Description
Implements version-specific handling for query insights settings to support API structure changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.