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

[OSCI] Remove unused tutorials #5212

Merged
merged 7 commits into from
Nov 7, 2023
Merged

Conversation

CMDWillYang
Copy link
Contributor

@CMDWillYang CMDWillYang commented Oct 4, 2023

Description

Removed unused tutorials from:
src/plugins/home/server/tutorials
src/plugins/home/public/assets/tutorials/logos

Issues Resolved

fixes #4448

Screenshot

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@CMDWillYang CMDWillYang changed the title WIP Remove unused tutorials [OSCI] WIP Remove unused tutorials Oct 4, 2023
@@ -36,7 +36,6 @@ import {
ScopedTutorialContextFactory,
} from './lib/tutorials_registry_types';
import { tutorialSchema } from './lib/tutorial_schema';
import { builtInTutorials } from '../../tutorials/register';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is an unused import, and also the only usage of tutorials under src/plugins/home/server/tutorials

Copy link
Member

Choose a reason for hiding this comment

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

@CMDWillYang Either in this PR, or as a standalone followup, can you also delete this comment:

// pre-populate with built in tutorials
// TODO: [RENAMEME] Need prod urls.
// https://github.com/opensearch-project/OpenSearch-Dashboards/issues/335
// this.tutorialProviders.push(...builtInTutorials);

@CMDWillYang
Copy link
Contributor Author

In the last commit, I removed unused svg assets under src/plugins/home/public/assets/tutorials/logos.

To verify the those were not used, I mainly searched the whole repo for usage of these logos, and I've found that those in the logos folder were not used(see some screenshots below).

Screenshot 2023-10-17 at 9 47 09 PM Screenshot 2023-10-17 at 9 43 49 PM

There does remain four png files under the tutorials folder (not tutorials/logos) that appear to be still used.
Screenshot 2023-10-17 at 9 54 16 PM

I know in the issue it's suggested to remove tutorials under src/plugins/home/public/application/components/tutorial. However, after some digging it appears most are still heavily used in other parts of the project, so I left those in for now.

If there's any suggestion on what more to remove, tips on how and where to search for additional used tutorials, or just feedback in general it would be greatly appreciated.

And of course, thanks so much to Josh for helping me here and at office hours to not completely drown in the sea of OpenSearch codebase :)

@CMDWillYang CMDWillYang marked this pull request as ready for review October 18, 2023 02:22
@CMDWillYang CMDWillYang changed the title [OSCI] WIP Remove unused tutorials [OSCI] Remove unused tutorials Oct 18, 2023
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #5212 (8761e15) into main (466d298) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #5212      +/-   ##
==========================================
- Coverage   66.81%   66.79%   -0.03%     
==========================================
  Files        3286     3286              
  Lines       63148    63148              
  Branches    10054    10054              
==========================================
- Hits        42191    42178      -13     
- Misses      18475    18486      +11     
- Partials     2482     2484       +2     
Flag Coverage Δ
Linux_1 35.24% <ø> (ø)
Linux_2 55.25% <ø> (ø)
Linux_3 43.82% <ø> (ø)
Linux_4 35.34% <ø> (ø)
Windows_1 ?
Windows_2 55.22% <ø> (ø)
Windows_3 ?
Windows_4 35.34% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...me/server/services/tutorials/tutorials_registry.ts 66.66% <ø> (ø)

... and 3 files with indirect coverage changes

joshuarrrr
joshuarrrr previously approved these changes Oct 18, 2023
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

LGTM! Minor suggestion on the changelog entry.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Josh Romero <[email protected]>
Signed-off-by: CMDWillYang <[email protected]>
@willie-hung
Copy link
Contributor

LGTM as well!

@joshuarrrr
Copy link
Member

I know in the issue it's suggested to remove tutorials under src/plugins/home/public/application/components/tutorial. However, after some digging it appears most are still heavily used in other parts of the project, so I left those in for now.

If there's any suggestion on what more to remove, tips on how and where to search for additional used tutorials, or just feedback in general it would be greatly appreciated.

@CMDWillYang Can you open a new issue for discussing what to do with these? I agree that we should probably handle them separately, and can add some additional context.

@joshuarrrr joshuarrrr merged commit 3c0211b into opensearch-project:main Nov 7, 2023
69 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 7, 2023
* removed unused content under src/plugins/home/server/tutorials

Signed-off-by: CMDWillYang <[email protected]>

* removed unused svg assets under src/plugins/home/public/assets/tutorials/logos

Signed-off-by: CMDWillYang <[email protected]>

* updated changelog

Signed-off-by: CMDWillYang <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Josh Romero <[email protected]>
Signed-off-by: CMDWillYang <[email protected]>

---------

Signed-off-by: CMDWillYang <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
(cherry picked from commit 3c0211b)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
manasvinibs pushed a commit that referenced this pull request Nov 8, 2023
* removed unused content under src/plugins/home/server/tutorials

Signed-off-by: CMDWillYang <[email protected]>

* removed unused svg assets under src/plugins/home/public/assets/tutorials/logos

Signed-off-by: CMDWillYang <[email protected]>

* updated changelog

Signed-off-by: CMDWillYang <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Josh Romero <[email protected]>
Signed-off-by: CMDWillYang <[email protected]>

---------

Signed-off-by: CMDWillYang <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
(cherry picked from commit 3c0211b)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
ananzh pushed a commit that referenced this pull request Nov 11, 2023
* removed unused content under src/plugins/home/server/tutorials
* removed unused svg assets under src/plugins/home/public/assets/tutorials/logos

---------

Signed-off-by: CMDWillYang <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
(cherry picked from commit 3c0211b)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unused tutorials
4 participants