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

Run Security dashboards plugin from binary #1726

Merged
merged 36 commits into from
Jan 16, 2024

Conversation

derek-ho
Copy link
Collaborator

@derek-ho derek-ho commented Jan 5, 2024

Description

Runs Opensearch dashboards from binary after installing a fresh build of security dashboards plugin. This should help catch issues not currently caught in our CI.

Category

[Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation]

Why these changes are required?

The purpose of this PR is to catch issues that may not be caught at development time. For example: when building the code, the front end code in the public folder will be compiled and bundled, the server code in the server folder will be compiled but not bundled. Thus, when you try to import a type from the public folder in the server folder, such as import { ResourceType } from '../../public/apps/configuration/types';, it wouldn't actually work with the built artifact. However, my current understanding is that since when you are developing locally this path actually exists, so thus this error was not caught. Running this workflow and doing a simple health check to verify that there was no fatal error in starting OSD should catch these issues.

Example of this new workflow catching previous errors: https://github.com/opensearch-project/security-dashboards-plugin/actions/runs/7543546304/job/20534811372

More about bundling here: https://snipcart.com/blog/javascript-module-bundler

What is the old behavior before changes and new behavior after changes?

Issues Resolved

Fix: #1709

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

  • New functionality includes testing
  • 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.

Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b5f7134) 67.09% compared to head (623dcb3) 67.09%.

❗ Current head 623dcb3 differs from pull request most recent head 7932eb6. Consider uploading reports for the commit 7932eb6 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1726   +/-   ##
=======================================
  Coverage   67.09%   67.09%           
=======================================
  Files          94       94           
  Lines        2404     2404           
  Branches      318      318           
=======================================
  Hits         1613     1613           
  Misses        713      713           
  Partials       78       78           

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

@derek-ho
Copy link
Collaborator Author

derek-ho commented Jan 8, 2024

Verified that this would have caught the types error we saw recently: https://github.com/opensearch-project/security-dashboards-plugin/actions/runs/7450037673/job/20268031021?pr=1726

Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

Thank you @derek-ho! I left a couple of comments. I'd like to understand when the min distribution of OSD core to published to see if it has the freshest changes or if there is a risk that this repo would be integrating with stale artifacts.

.github/actions/install-dashboards/action.yml Outdated Show resolved Hide resolved
.github/actions/install-dashboards/action.yml Outdated Show resolved Hide resolved
@derek-ho
Copy link
Collaborator Author

derek-ho commented Jan 8, 2024

Thank you @dxho! I left a couple of comments. I'd like to understand when the min distribution of OSD core to published to see if it has the freshest changes or if there is a risk that this repo would be integrating with stale artifacts.

That link is related to the latest build /latest/ will be replaced by the build id of the latest. I tried building OSD-core prior to this commit: 48af998. IMHO that is also valid, but after thinking it through, we should not be responsible for the build process of core, and instead rely on the build team for that. If there is an issue with core, both would fail anyway, so we should just rely on the build team.

@cwperks
Copy link
Member

cwperks commented Jan 8, 2024

For core we download the following artifacts:

Windows: https://artifacts.opensearch.org/snapshots/core/opensearch/${{ inputs.opensearch-version }}-SNAPSHOT/opensearch-min-${{ inputs.opensearch-version }}-SNAPSHOT-windows-x64-latest.zip

Linux: https://artifacts.opensearch.org/snapshots/core/opensearch/${{ inputs.opensearch-version }}-SNAPSHOT/opensearch-min-${{ inputs.opensearch-version }}-SNAPSHOT-linux-x64-latest.tar.gz

These artifacts are published after PRs are merged into the release branches in the core repo. Integrating with the latest commits in Core or OSD Core help uncover issues quickly so I think it makes sense to aim for integrating with the most up-to-date changes if its possible.

Copy link
Collaborator

@RyanL1997 RyanL1997 left a comment

Choose a reason for hiding this comment

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

Hi @derek-ho, thanks for taking this on. I know this is still the draft version, but I just left some early thoughts on this.

.github/workflows/cypress-test-tenancy-disabled.yml Outdated Show resolved Hide resolved
.github/workflows/cypress-test.yml Outdated Show resolved Hide resolved
.github/workflows/cypress-test-oidc-e2e.yml Outdated Show resolved Hide resolved
.github/actions/install-dashboards/action.yml Outdated Show resolved Hide resolved
.github/actions/install-dashboards/action.yml Outdated Show resolved Hide resolved
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Nice work, a couple of comments inline - before we merge, I'd also like to see a set of 10x runs of all jobs so we can be sure they have good stability

.github/workflows/cypress-test-tenancy-disabled.yml Outdated Show resolved Hide resolved
.github/actions/install-dashboards/action.yml Outdated Show resolved Hide resolved
.github/workflows/cypress-test.yml Outdated Show resolved Hide resolved
.github/workflows/cypress-test-oidc-e2e.yml Outdated Show resolved Hide resolved
.github/actions/install-dashboards/action.yml Outdated Show resolved Hide resolved
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
@derek-ho derek-ho changed the title Run Security dashboards plugin from binary for integration tests Run Security dashboards plugin from binary Jan 16, 2024
Signed-off-by: Derek Ho <[email protected]>
…rver from public folder (opensearch-project#1705)""

This reverts commit 2de58e3.

Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Copy link
Contributor

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

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

Overall looks good to me just one spot where I am not sure if an ambersand is needed or not.

@derek-ho derek-ho added the backport 2.x backport to 2.x branch label Jan 16, 2024
Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

thanks @derek-ho !

@DarshitChanpura DarshitChanpura merged commit 9b88d92 into opensearch-project:main Jan 16, 2024
13 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 16, 2024
Signed-off-by: Derek Ho <[email protected]>
(cherry picked from commit 9b88d92)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
6 participants