Updates Dev guide

[DarshitChanpura]

Description

Enhances the dev-guide to be more elaborative and intuitive.

Category

Documentation

Check List

• 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.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.21%. Comparing base (bc61403) to head (ed5c07d).
⚠️ Report is 295 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #897   +/-   ##
=======================================
  Coverage   72.21%   72.21%           
=======================================
  Files          87       87           
  Lines        1915     1915           
  Branches      249      249           
=======================================
  Hits         1383     1383           
  Misses        478      478           
  Partials       54       54           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[davidlago]

Probably a typo (is as a plugin)

[DarshitChanpura]

fixed it

[davidlago]

I'd probably make this relative to .node-version so we don't have to upgrade every time (I'm not sure if we already use a .node-version file but we should)

[DarshitChanpura]

ahaa...yes...we do use .node-version and .nvm-rc.. I just added an extra explanation saying "refer to .node-version file in base directory"

[pc-jedi]

Suggested change

	nvm install 10.24.1
	nvm use --install

That way we don't have to specify the version explicitly, since it is picked up from .nvmrc

[cliu123]

What's the deal with this comment?

[pc-jedi]

Just an idea for an improvement on the docs. E.g. if we move towards 2.0 the node version changed and that would mean we always have to update the docs when we change .nvmrc. If we simply use the nvm use --install command we then don't have to upgrade the docs all the time.

[hsiang9431-amzn]

Are we updating this statement every time the branch is changed?

[davidlago]

L14 says this is used just during the guide as just an example. The paragraph above makes reference to the need for matching versions. But yeah, if we move to a major version (2, 3...) it might be a bit confusing so we can update. Perhaps we can omit L14 as L10 covers the general idea and then we move on to use 1.3.0 as a driving example (that part can stay as it is clear it's just for the sake of the example code)

[hsiang9431-amzn]

I think down the road when there are multiple versions and branches for developers to choose, adding a table to show each version with there corresponding branch will be helpful

[DarshitChanpura]

I have added the relevant branch and version info here... we can add info to this table as we develop in the future

[DarshitChanpura]

@hsiang9431-amzn @davidlago ... can you please review this again?

[cliu123]

ITs failed. Would you please take a look?

[cliu123]

Is there a template reference of the Dev Guide?
Please sign the commits. All commits are "Unverified".

[DarshitChanpura]

Is there a template reference of the Dev Guide? Please sign the commits. All commits are "Unverified".

I did sign all the commits idk why it says Unsigned...Shouldn't DCO check fail if commits are unsigned?

[DarshitChanpura]

ITs are failing because the integration test uses 1.x branch as base for OpenSearch-Dashboards (see here)

Because 1.x has been bumped to 1.4 in the core dashboards repo, there is a mismatch between plugin's expected version (1.4.0.0) and actual version (1.3.0.0) when integration tests are run. This results in failing ITs.

Error log from one of the failing tests:

start OpenSearch Dashboards server › call multitenancy info API as admin

    Failed to initialize plugins:
    	Plugin "securityDashboards" is only compatible with OpenSearch Dashboards version "1.3.0", but used OpenSearch Dashboards version is "1.4.0". (incompatible-version, /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/OpenSearch-Dashboards/plugins/security-dashboards-plugin/opensearch_dashboards.json)

      214 |       .toPromise();
      215 |     if (errors.length > 0) {
    > 216 |       throw new Error(
          |             ^
      217 |         `Failed to initialize plugins:${errors.map((err) => `\n\t${err.message}`).join('')}`
      218 |       );
      219 |     }

      at PluginsService.handleDiscoveryErrors (src/core/server/plugins/plugins_service.ts:216:13)

This change will be updated to point to main in 2.0 version bump PR: #928

[DarshitChanpura]

I would suggest waiting until main branch is bumped to 2.0 and then rebasing this branch with main before merging it.

[cliu123]

@DarshitChanpura main branch has been bumped to 2.1.0.0 now. Do you want to rebase and get it ready for review?

[DarshitChanpura]

@opensearch-project/security Can I get review on this?

[peternied]

While I think it would be better to use more recent builds, IMO any update to this guide that make it more usable are for the better, and we can update it afterward.