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

Remove legacy dependency #702

Merged
merged 2 commits into from
Mar 27, 2024
Merged

Remove legacy dependency #702

merged 2 commits into from
Mar 27, 2024

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented Mar 4, 2024

Description

Remove a legacy dependency not needed by plugins anymore. The consequence is it removes a huge amount of transitive dependencies in yarn.lock, many of which are dated and frequently cause CVE-related issues.

Testing done

  • yarn build works
  • yarn osd bootstrap works
  • sanity test on a local domain

Still understanding other potential impacts and original reasoning for having this dependency, will update here when more is learned from @opensearch-project/opensearch-core team. UPDATE: this has been confirmed by @kavilla , no other concerns.

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

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.

@ohltyler
Copy link
Member Author

ohltyler commented Mar 4, 2024

@opensearch-project/opensearch-dashboards-core could I get assistance in understanding why this dependency is not needed anymore? My understanding is it was needed somewhere in the build or bootstrap logic for combining dependencies from core + plugin to build/run the plugin successfully. Is there any reference PR in core that I could reference here?

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.31%. Comparing base (0e713e0) to head (d921dcb).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #702   +/-   ##
=======================================
  Coverage   50.31%   50.31%           
=======================================
  Files         166      166           
  Lines        5593     5593           
  Branches     1074     1074           
=======================================
  Hits         2814     2814           
  Misses       2508     2508           
  Partials      271      271           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dbwiddis
dbwiddis previously approved these changes Mar 12, 2024
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

Nice.

Can we just completely delete yarn.lock and add it to .gitignore since it's always re-generated at run time anyway? Or would that prevent the CVE scanners from figuring out what needs updating?

@ohltyler
Copy link
Member Author

Nice.

Can we just completely delete yarn.lock and add it to .gitignore since it's always re-generated at run time anyway? Or would that prevent the CVE scanners from figuring out what needs updating?

We still want the lockfile, and with this fix it should not be re-generated often; only when we make direct changes in our package.json. The reason it was much more fluid before is the fact it was pulling in updated dependencies from the tip of main of core OSD.

@ohltyler
Copy link
Member Author

@kavilla any further insight on this?

@@ -23,7 +23,6 @@
]
},
"devDependencies": {
"@elastic/eslint-import-resolver-kibana": "link:../../packages/osd-eslint-import-resolver-opensearch-dashboards",
Copy link
Member

Choose a reason for hiding this comment

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

I would modify: https://github.com/opensearch-project/anomaly-detection-dashboards-plugin/blob/main/.eslintrc.yml#L6 as well.

From my understanding this dependency has it's use cases but I don't see it being used within this plugin repo that makes it worth linking.

If this was a shared repo in the past, or if you need to extend the imports rules then I get see it. But after removing and modifying the eslintrc, I needed to modify the import orders as I believe our lint rules kicked in for import order and it didn't treat your plugin as the root directory

Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
@ohltyler
Copy link
Member Author

ohltyler commented Mar 26, 2024

Removed lintfile, re-did a bunch of sanity checks, looks good.

yarn osd bootstrap succeeds

UT:

Test Suites: 1 skipped, 76 passed, 76 of 77 total
Tests:       2 skipped, 364 passed, 366 total
Snapshots:   70 passed, 70 total
Time:        98.704 s
Ran all test suites.
✨  Done in 100.18s.

Build:

yarn build
yarn run v1.22.19
$ yarn plugin-helpers build && echo Renaming artifact to $npm_package_config_plugin_zip_name-$npm_package_config_plugin_version.zip && mv ./build/$npm_package_config_plugin_name*.zip ./build/$npm_package_config_plugin_zip_name-$npm_package_config_plugin_version.zip
$ node ../../scripts/plugin_helpers build
 info Loaded config file from [/Users/ohltyler/workspace/opensearch/ad/3.0/OpenSearch-Dashboards/plugins/anomaly-detection-dashboards-plugin-1/.opensearch_dashboards-plugin-helpers.json]
 info deleting the build and target directories
 info running @osd/optimizer
 │ info initialized, 0 bundles cached
 │ info starting worker [1 bundle]
 │ succ 1 bundles compiled successfully after 41 sec
 info copying assets from `public/assets` to build
 info copying server source into the build and converting with babel
 info running yarn to install dependencies
 info compressing plugin into [anomalyDetectionDashboards-3.0.0.zip]
 info cleaning up compression temporary artifacts
Renaming artifact to 

Sanity tested functionality and checked all pages on local cluster, no problems.

@ohltyler ohltyler requested a review from dbwiddis March 27, 2024 00:40
@amitgalitz
Copy link
Member

I see some failures in plugin bootstrap in CI, should we address these?

@ohltyler
Copy link
Member Author

I see some failures in plugin bootstrap in CI, should we address these?

CI passes for linux. The failure is some timeout issue specific to windows, I believe this is an overall infra issue across the project, needs further investigation. Remote integ test failure is being addressed by @jackiehanyang , Jackie, can you include this in your investigation?

Prefer to keep the scope the same for this PR.

@ohltyler ohltyler merged commit b10271c into opensearch-project:main Mar 27, 2024
7 of 11 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 27, 2024
Signed-off-by: Tyler Ohlsen <[email protected]>
(cherry picked from commit b10271c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants