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

Reuse of Opensearch core and Opensearch Dashboard setup for CI workflow #1212

Merged

Conversation

RyanL1997
Copy link
Collaborator

Signed-off-by: Ryan Liang [email protected]

Description

Reuse of Opensearch core and Opensearch Dashboard setup for CI workflow

Category

Enhancement

Issues Resolved

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.

@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2022

Codecov Report

Merging #1212 (1f9d42b) into main (e15b6fc) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1212   +/-   ##
=======================================
  Coverage   71.78%   71.78%           
=======================================
  Files          88       88           
  Lines        2027     2027           
  Branches      269      269           
=======================================
  Hits         1455     1455           
  Misses        509      509           
  Partials       63       63           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@RyanL1997 RyanL1997 force-pushed the sec-dashboards-github-action branch 7 times, most recently from f38c661 to 02096df Compare November 12, 2022 06:41
.github/workflows/cypress-test.yml Outdated Show resolved Hide resolved
.github/actions/install-dashboards/action.yml 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
.github/workflows/cypress-test.yml Show resolved Hide resolved
.github/workflows/cypress-test.yml Show resolved Hide resolved
@RyanL1997 RyanL1997 force-pushed the sec-dashboards-github-action branch 2 times, most recently from eb8f8f0 to 1204f07 Compare November 29, 2022 17:44
@RyanL1997 RyanL1997 force-pushed the sec-dashboards-github-action branch 15 times, most recently from ba684e4 to be0f521 Compare November 30, 2022 17:35
@stephen-crawford
Copy link
Contributor

stephen-crawford commented Dec 1, 2022

I have been working on this as well to try to lend a hand. I do not think that GitHub Actions will support removing the manual code any further than what @RyanL1997 has already done.

As things are, GitHub actions has two things that prevent us from doing what we want to do. 1) The checkout action always operates from the working directory and always cleans the entire directory when checking out. This means it is impossible to checkout the security-plugin repo during the security action file without a direct implementation of the action in the security-dashboards repo. So when you call the security repo action the checkout action in the workflow will always move back to the working directory (security-dashboards/security-dashboards) and then wipe that directory and download the files from the security repo. This can be worked around by navigating directories or swapping to a reusable workflow setup (https://github.com/scrawfor99/security-dashboards-plugin/tree/sec-dash-action). It does make some of the work Ryan has been doing tricky however.

The second issue is even more problematic and cannot be worked around as far as I have found. The checkout action will always execute commands from a checkout repository in the context of the calling repo NOT the called repo (https://docs.github.com/en/actions/using-workflows/reusing-workflows). This means we cannot call gradle build to get the built plugin zip necessary for installing the plugin into opensearch core. Exacerbating this issue, copying the plugin install action into the dashboards repo will not work because of the lack of gradle properties in the dashboards repo.

Ultimately, I believe that Ryan's best option is to just revert back to what he had working and then we merge it. Once GHA supports an alternative form of checkout which can execute script from the called repo's context it should be a quick swap over to the desired sleek design.

@RyanL1997
Copy link
Collaborator Author

@peternied @cwperks Here is the update of reusing the security repo's action:

I was trying to solved this error by manually downloading the security plugin and manually creating the directory ./build/distribution[1] to adapt this command in security repo's action. However, this method doesn't work since the checkout step will wipe out the directory, so that we are constantly receiving this error during the checkout step. I also double confirmed with @scrawfor99, and this is the expected behavior for github actions.

Reference:
[1]: https://github.com/opensearch-project/security-dashboards-plugin/actions/runs/3587095716/workflow#L31-L39

@cwperks
Copy link
Member

cwperks commented Dec 1, 2022

@RyanL1997 Would reversing the order of checking out the branch and downloading the zip provide a temporary fix?

I think it would make sense to add a boolean param with default of false to the start-opensearch-with-one-plugin action (maybe called download_plugin_zip) to download the zip of the plugin from ci.opensearch.org. If the param is set to false it would indicate that the plugin zip is provided to the action, usually as the output of ./gradlew assemble of the checked out branch.

i.e.

in https://github.com/opensearch-project/security/blob/main/.github/actions/start-opensearch-with-one-plugin/action.yml

# Download the plugin for installation
  - name: Download the plugin for installation
    if: ${{inputs.download-plugin-zip}} == true
    run: <download_logic_here>
    shell: bash

# Move and rename the plugin for installation
- name: Move and rename the plugin for installation
  if: ${{inputs.download-plugin-zip}} == false
  run: mv ./build/distributions/${{ inputs.plugin-name }}-*.zip ${{ inputs.plugin-name }}.zip
  shell: bash

@peternied
Copy link
Member

peternied commented Dec 1, 2022

@RyanL1997 @scrawfor99 I think the checkout issues was a red herring - I was able to use a slightly modified version of the job and it looks like it nearly mostly worked [1]. The outstanding failures were test issues that I believe are related to this line echo "plugins.security.unsupported.restapi.allow_securityconfig_modification: true" >> /opensearch/config/opensearch.yml
Checkout what I did here

[1] https://github.com/peternied/security-dashboards-plugin/actions/runs/3594671289/jobs/6053224787

@stephen-crawford
Copy link
Contributor

@peternied @RyanL1997 this looks great. I am glad you managed to avoid the checkout problem altogether with this workaround. @peternied do we need to modify the base for the action merged into the security repo or just call yours? Either seems fine but I will make the changes to security if we want to go that route so that Ryan does not have to.

@RyanL1997 RyanL1997 force-pushed the sec-dashboards-github-action branch 5 times, most recently from efe919c to b486c56 Compare December 1, 2022 20:17
@RyanL1997
Copy link
Collaborator Author

@scrawfor99 I'm testing with my own fork rn for the security repo side. I will let you know or I can do it after I get it to work.

@RyanL1997 RyanL1997 force-pushed the sec-dashboards-github-action branch from b486c56 to 2b88141 Compare December 1, 2022 20:49
@peternied
Copy link
Member

do we need to modify the base for the action merged into the security repo

Please fix the opensearch-project/security action. I appreciate the confidence in my fork :D

@RyanL1997
Copy link
Collaborator Author

RyanL1997 commented Dec 1, 2022

@peternied @cwperks @scrawfor99 Thank you all for the input! I just fixed the workflow by adding the changes (opensearch-project/security@main...RyanL1997:security:security-reusable-action). If we are good with this change, I can create a PR in security repo to apply it. Once it gets merged, I can change the reference from my own fork to Security repo and make this PR ready for review.

@RyanL1997
Copy link
Collaborator Author

I just created the fix PR in Security: opensearch-project/security#2290

Signed-off-by: Ryan Liang <[email protected]>
@RyanL1997 RyanL1997 force-pushed the sec-dashboards-github-action branch from d457a55 to be2ba5b Compare December 2, 2022 19:36
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.

Nicely done! Lets get this merged so we can make all these test systems run on linux and windows!



- name: Run OpenSearch with plugin
run: wget --progress=bar:force:noscroll -O opensearch-security.zip https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/${{ env.OPENSEARCH_VERSION }}/latest/linux/x64/tar/builds/opensearch/plugins/${{ env.PLUGIN_NAME }}-${{ env.PLUGIN_VERSION }}.zip
Copy link
Member

Choose a reason for hiding this comment

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

why --progress=bar:force:noscroll here?

Copy link
Collaborator Author

@RyanL1997 RyanL1997 Dec 3, 2022

Choose a reason for hiding this comment

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

Hi @DarshitChanpura thanks for the review! We use this flag because otherwise the log will look like this https://github.com/RyanL1997/security-dashboards-plugin/actions/runs/3576589551/jobs/6014577673#step:7:837. This one is so hard to track.

Copy link
Member

Choose a reason for hiding this comment

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

Lets replace this with peternied/download-file@v2 in a fast-following PR, as these actions should support multi-platform which it does not

.github/workflows/cypress-test.yml Show resolved Hide resolved
@stephen-crawford
Copy link
Contributor

@RyanL1997 Awesome job here!

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.

Great work @RyanL1997! 🚢

@RyanL1997 RyanL1997 merged commit 0c07eaf into opensearch-project:main Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maintenance] Enable CI Workflows to Re-use Setup Steps
7 participants