-
Notifications
You must be signed in to change notification settings - Fork 55
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
GettingStarted Rework #2194
GettingStarted Rework #2194
Conversation
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
public/components/getting_started/components/getting_started.tsx
Outdated
Show resolved
Hide resolved
@@ -108,88 +108,45 @@ | |||
} | |||
], | |||
"getting-started": { | |||
"info": "https://github.com/opensearch-project/opensearch-catalog/blob/main/integrations/observability/nginx/getting-started/Getting-Started.md", | |||
"info": { |
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.
where's the source of these json files, do we have to main all these files?
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.
This link is not exposed to the user and was previously used to generate information on the dashboard we are not creating anymore.
We are currently manually making all these json's to provide the steps/workflows but would like to eventually move these to the catalog.
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.
This link is not exposed to the user and was previously used to generate information on the dashboard we are not creating anymore.
We've spoke about this offline, this is not specifically for this info but overall like do we have to manually create/maintain these json files on the UI side.
We are currently manually making all these json's to provide the steps/workflows but would like to eventually move these to the catalog.
Could we create issue for this just for tracking purposes?
public/components/getting_started/getting_started_artifacts/otel-services/.DS_Store
Outdated
Show resolved
Hide resolved
@@ -98,6 +99,9 @@ export const Home = (props: HomeProps) => { | |||
return ( | |||
<div> | |||
{dataSourceMenuComponent} | |||
<HeaderControlledComponentsWrapper | |||
description={'Get started with collecting and monitoring your observability data.'} |
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 move this to constant file and add i18n
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.
Added i18n
<HeaderControlledComponentsWrapper
description={i18n.translate('observabilityGetStarted.description', {
defaultMessage: 'Get started with collecting and monitoring your observability data.',
})}
I don't think it should be moved to constant folder as it is a 1 off description and currently all the top nav registration of descriptions are done in file.
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.
thanks, generally we should for better maintainability but not a blocker
height: 40px; |
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.
These styles should be moved to corresponding component scss file(s), here should only contain general import as this is the index file
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.
The icon components are shared between integrations and getting-started to keep the image size consistent between the plugins.
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.
you could reuse existing or create new shared scss files for general styings that are shared across different modules. index file here are for modularity importing of scss partials.
@@ -8,8 +8,12 @@ import path from 'path'; | |||
|
|||
export const assetMapper = (tutorialId: string) => { |
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.
We may want to consider refactor this code piece as it is
- not easy to maintain, the growing number of cases makes these magic strings
- proven to typos
- not easy to be modified when naming and version changes along the way
we want to consider
- use map or enum for all these file names and move these stuff to constant file.
type TutorialId = 'otelLogs' | 'otelMetrics' | 'otelTraces' | 'nginx' | 'java' | 'python' | 'golang';
const assetMap: Record<TutorialId, string> = {
otelLogs: 'otel-index-patterns-1.0.0-Logs.ndjson',
otelMetrics: 'otel-index-patterns-1.0.0-Metrics.ndjson',
otelTraces: 'otel-index-patterns-1.0.0-Traces.ndjson',
nginx: 'nginx-1.0.0.ndjson',
java: 'java-tutorial-1.0.0.ndjson',
python: 'python-tutorial-1.0.0.ndjson',
golang: 'golang-tutorial-1.0.0.ndjson',
};
export const assetMapper = (tutorialId: TutorialId): string => {
return assetMap[tutorialId] || '';
};
- maybe use concatenations like
${component}-${indexPattens}-${version}-${signal}.ndjson
- provides a default return?
Also similar question: are these files coming from integration? If so, do we need to maintain these files from within get started module? Can't we just fetch from somewhere?
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.
These are all currently manually created.
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.
Adjusted it to be
export const assetMapper = (tutorialId: TutorialId): string => {
const component = COMPONENT_MAP[tutorialId] || 'default-component';
const version = VERSION_MAP[tutorialId] || '1.0.0';
const signal = SIGNAL_MAP[tutorialId] ? `-${SIGNAL_MAP[tutorialId]}` : '';
return `${component}-${version}${signal}.ndjson`;
};
and moved others to constants.
Could we also add tests to some of these changes? |
Signed-off-by: Adam Tackett <[email protected]>
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/dashboards-observability/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/dashboards-observability/backport-2.x
# Create a new branch
git switch --create backport/backport-2194-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f68eba1c45920df2866875dd5542cfe805e3f241
# Push it to GitHub
git push --set-upstream origin backport/backport-2194-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/dashboards-observability/backport-2.x Then, create a pull request where the |
* UI pass updating everything Signed-off-by: Adam Tackett <[email protected]> * dashboard re-direct to integration for otel Signed-off-by: Adam Tackett <[email protected]> * correct naming and icon for three cases Signed-off-by: Adam Tackett <[email protected]> * updates to jsons and ndjsons Signed-off-by: Adam Tackett <[email protected]> * remove dashboard creation from the create assets call Signed-off-by: Adam Tackett <[email protected]> * update name to Self Managed Signed-off-by: Adam Tackett <[email protected]> * Remove hyperlink from discover Signed-off-by: Adam Tackett <[email protected]> * address comments Signed-off-by: Adam Tackett <[email protected]> --------- Signed-off-by: Adam Tackett <[email protected]> Co-authored-by: Adam Tackett <[email protected]> (cherry picked from commit f68eba1)
* UI pass updating everything Signed-off-by: Adam Tackett <[email protected]> * dashboard re-direct to integration for otel Signed-off-by: Adam Tackett <[email protected]> * correct naming and icon for three cases Signed-off-by: Adam Tackett <[email protected]> * updates to jsons and ndjsons Signed-off-by: Adam Tackett <[email protected]> * remove dashboard creation from the create assets call Signed-off-by: Adam Tackett <[email protected]> * update name to Self Managed Signed-off-by: Adam Tackett <[email protected]> * Remove hyperlink from discover Signed-off-by: Adam Tackett <[email protected]> * address comments Signed-off-by: Adam Tackett <[email protected]> --------- Signed-off-by: Adam Tackett <[email protected]> Co-authored-by: Adam Tackett <[email protected]> (cherry picked from commit f68eba1)
Description
Updates to GettingStarted
Video of new experience
https://github.com/user-attachments/assets/a8cc30d6-70a7-4d18-b8c3-6b7ffada0869
For Nginx
Redirect to
Issues Resolved
Check List
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.