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

Add remote cypress orchestrator integration with integtest script #988

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

manasvinibs
Copy link
Member

@manasvinibs manasvinibs commented Jan 6, 2024

Description

Adding remote cypress workflow orchestrator script integration with existing logic of integtest.sh script. This will keep the current behavior of integtest script as it is when triggered along with silently triggering the remote cypress workflows present in the plugin/components as defined in manifest json file and continue to poll them till its completion.

Changes summary:

  • Orchestrator integration with integtest script.
  • Adding manifest json file to define plugin/components that gets onboarded to use orchestrator workflow. This manifest is used to capture plugin repo name, workflow name, branch that needs to be tested and any additional security related configuration that needs to be passed to the cypress tests running remote github workflows.
  • In this iteration I'm adding Dashboards component to manifest but the workflow file and branch related information will be added once they are ready within Dashboards component.

Issues Resolved

Follow-up to
#972

#991

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.

@SuZhou-Joe
Copy link
Member

Can we have a demo log or something that we can take a look that can tell us how will the feature be like in the future?

@manasvinibs
Copy link
Member Author

Can we have a demo log or something that we can take a look that can tell us how will the feature be like in the future?

Certainly! Here is the link to my forked repository, opensearch-dashboards-functional-test, where I am currently executing the integtest.sh script through a GitHub workflow for testing purposes.
https://github.com/manasvinibs/opensearch-dashboards-functional-test/actions/runs/7427907888/job/20214377315

The Run Bash Script logs display the initiation of the remote Cypress GitHub workflow and its continuous polling until completion. The existing logic within the integtest.sh script, designed for running in-house Cypress tests, now accommodates the wait for the remote workflow to finish before executing the remaining logic at the end. Consequently, in this scenario, an expected Cypress error is anticipated due to the absence of a configured Cypress server locally, a condition that occurs towards the conclusion of the process.

integtest.sh Outdated
Comment on lines 132 to 137
os_url=$(echo "$component" | jq -r '.["opensearch"]')
osd_url=$(echo "$component" | jq -r '.["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.

What about using the
https://artifacts.opensearch.org/releases/bundle/opensearch/{version_from_manifest}/opensearch-{version_from_manifest}-linux-x64.tar.gz

https://artifacts.opensearch.org/releases/bundle/opensearch-dashboards/{version_from_manifest}/opensearch-dashboards-{version_from_manifest}-linux-x64.tar.gz

as fallbacks of os_url and osd_url?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! I think OS and OSD url could be other platform/distribution combination as well in addition to linux-x64.tar. But I think we can default to linux-x64.tar when we don't have one defined in manifest. Updated in the latest revision.

"schema-version": "1.0",
"build": {
"name": "OpenSearch Dashboards Functional Test",
"version": "2.12.0"
Copy link
Member

Choose a reason for hiding this comment

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

On main the version should be 3.0.0 I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right. Updated.

Comment on lines 15 to 18
"workflow-name": "",
"ref": "",
"opensearch": "test",
"opensearch-dashboards": "test",
Copy link
Member

Choose a reason for hiding this comment

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

As this is an open PR, can we modify these dummy configs to valid ones?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes for main I have added linux-x64 artifact for now. Release team can choose to update component manifest json for other artifact distribution as needed.

integtest.sh Outdated
done

# Wait for all processes to finish
wait "${all_process_pids[@]}"
Copy link
Member

Choose a reason for hiding this comment

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

Can we make remote cypress tasks and FTRepo test task parallel and wait for both to finish? Or I think FTRepo's task will wait for a long time to start.

Copy link
Member Author

Choose a reason for hiding this comment

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

We wanted to introduce this feature silently integrating in the background of existing integtest script runner for the first iteration to observe the orchestrator behavior. By keeping FTRepo test task run in the foreground after wait, we are not changing the behavior in terms of response code of the integtest script completion. We can make it run in background parallel to other remote cypress tasks, when we are ready to read the response codes from all the tasks integrated and send to integtest script caller (infra team jenkins job).
I agree there will be delay in the start of the FTRepo task but this will help in to ensure better control, immediate visibility into the results, and simplified error handling. This approach aligns with our current requirements and allows us to seamlessly integrate the remote test results into subsequent processes. Let me know if you have other thoughts.

@ruanyl
Copy link
Member

ruanyl commented Jan 10, 2024

@manasvinibs Is there design doc or RFC of this change? I'd like to understand the motivation and long term vision of introducing this feature, it seems this is just the first step :)

Found this issue: opensearch-project/OpenSearch-Dashboards#5392

"version": "2.12.0"
},
"ci": {
"image": {
Copy link
Member

Choose a reason for hiding this comment

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

Q: how will this image be used?

Copy link
Member Author

Choose a reason for hiding this comment

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

This manifest value is optional for now and serves to specify a container image where tests are run , as required by the Orchestrator caller (release workflow). It becomes relevant when we aim to ensure not only the execution of tests on specific version/artifacts but also the use of a specific image configured by the release team. The Orchestrator passes this information to the remote Cypress workflows running in the components/plugins, allowing them to align their GitHub workflows with the image provided by the Orchestrator. While not mandatory, it provides added flexibility to the Orchestrator, allowing it to dictate the container image if/when necessary.

remoteCypress.sh Outdated
# Check if required arguments are provided
if [[ -n "$REPO" && -n "$WORKFLOW_NAME" && -n "$OS_URL" && -n "$OSD_URL" && -n "$BRANCH_REF" ]]; then
# Accessing the secret as an environment variable using Github actions while invoking this script
GITHUB_TOKEN=$GITHUB_SECRET_TOKEN
Copy link
Member

Choose a reason for hiding this comment

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

Is GITHUB_SECRET_TOKEN a newly added env var or existing one? Which party is maintaining it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes GITHUB_SECRET_TOKEN is newly added env variable part of the github workflow - https://github.com/opensearch-project/opensearch-dashboards-functional-test/blob/main/.github/workflows/remote-cypress-workflow.yml#L33 but the token used is existing one within the team's repo used by other github workflows such as delete workflow

Copy link
Member

Choose a reason for hiding this comment

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

I see, is there cases that this script is run outside of github workflow where GITHUB_TOKEN is not set?

Copy link
Member Author

@manasvinibs manasvinibs Jan 22, 2024

Choose a reason for hiding this comment

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

Currently, this will be run as part of integtest script. Along with that one can manually trigger the remoteCypress script using dispatch event. When integtest script is triggered as part of jenkins job from infra team, Github token is already set in Jenkins environment which is accessed within this script when run in the jenkins environment.

integtest.sh Outdated
source remoteCypress.sh -r "$repo" -w "$workflow_name" -o "$os_url" -d "$osd_url" -b "$branch_ref" &
bg_process_pid=$!
echo "PID for the repo $repo is : $bg_process_pid"
all_process_pids+=($bg_process_pid)
Copy link
Member

Choose a reason for hiding this comment

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

i think it might be worth to consider using pid files https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/config/opensearch_dashboards.yml#L122

i didn't realize this logic existed but has long as the file is created we can reference the process id file instead of it in memory

integtest.sh Outdated
REMOTE_MANIFEST_FILE="remote_cypress_manifest.json"

# Parse the JSON file using jq and iterate over the components array
components=$(jq -c '.components[]' "$REMOTE_MANIFEST_FILE")
Copy link
Member

Choose a reason for hiding this comment

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

nit: for readability it might be helpful to have a helper function for this and perhaps it returns back a tuple with the required information

integtest.sh Outdated
branch_ref=$(echo "$component" | jq -r '.["ref"]')

# Set default values if the opensearch and opensearch-dashboards are not set in the manifest
os_url=${os_url:-https://artifacts.opensearch.org/releases/bundle/opensearch/$release_version/opensearch-$release_version-linux-x64.tar.gz}
Copy link
Member

Choose a reason for hiding this comment

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

linux-x64. i think it should get the operating system of the runner and arch

Copy link
Member Author

Choose a reason for hiding this comment

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

I've adjusted the manifest to get operating system/arch and constructing the url while parsing the manifest with the base url being https://ci.opensearch.org/ci/dbc/distribution-build/.

integtest.sh Outdated
echo "branch_ref: $branch_ref"

# Call the function for each component
run_remote_cypress "$repo" "$workflow_name" "$os_url" "$osd_url" "$branch_ref"
Copy link
Member

Choose a reason for hiding this comment

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

i think we should add a flag or env variable that enables us to conditional run the run remote cypress tests are not. since this repo is now being locked as well to commits a new build will have to be made if there are any issues. so it pre-planning a feature flag could be a fallback to disabling this if it causes issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

I referenced the developer guide doc about setting up flag for experimental features and its tests. As my feature is not actually running cypress tests but instead running orchestrator workflows, I've introduced a simple boolean flag as part of integtest.sh script params defaulting to true. Build script which triggers integtest.sh can set the feature flag to true/false as required. Is this the behavior you were expecting or is there some other way to configure this setting in FTRepo?

@manasvinibs manasvinibs force-pushed the Issue-972-integration branch 2 times, most recently from 7b55235 to cb4644a Compare January 29, 2024 19:53
Signed-off-by: manasvinibs <manasvis@amazon.com>
@manasvinibs
Copy link
Member Author

@kavilla @SuZhou-Joe @ruanyl Can you please review and let me know if there anything else you would like me to address to get this checked in? I'm hoping to get this in for 2.12 and would like to have some time to bake this changes in main and address any issues related to integration.

"repository": "opensearch-project/OpenSearch-Dashboards",
"workflow-name": "",
"ref": "",
"operating-system": "linux",
Copy link
Member

Choose a reason for hiding this comment

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

os

done < "$pid_file"
}

if [[ $REMOTE_CYPRESS_ENABLED = "true" && $ORCHESTRATOR_FEATURE_FLAG = 'true' ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

check if env variable for github token if not then don't execute this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! I have a follow-up PR to add manifest data to this. I'll address this one in that.

@ananzh ananzh merged commit a844c23 into opensearch-project:main Feb 2, 2024
38 checks passed
peterzhuamazon added a commit to peterzhuamazon/opensearch-dashboards-functional-test that referenced this pull request Feb 6, 2024
peterzhuamazon added a commit that referenced this pull request Feb 6, 2024
…backports and disable windows video recording (#1070)

* [2.x] Adding placeholder to keep main/2.x/1.x consistent before #988 backports

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Add more

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

---------

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 6, 2024
…backports and disable windows video recording (#1070)

* [2.x] Adding placeholder to keep main/2.x/1.x consistent before #988 backports

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Add more

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

---------

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
(cherry picked from commit 4add696)
peterzhuamazon added a commit that referenced this pull request Feb 6, 2024
…backports and disable windows video recording (#1070) (#1071)

* [2.x] Adding placeholder to keep main/2.x/1.x consistent before #988 backports

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Add more

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

---------

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
(cherry picked from commit 4add696)

Co-authored-by: Peter Zhu <zhujiaxi@amazon.com>
@peterzhuamazon
Copy link
Member

peterzhuamazon commented Feb 6, 2024

Hi @manasvinibs if you ever want to backport please note that I have add placeholder for -r in case 1.x/2.x wont fail when we call -r false.
#1070

We have to disable the remote test settings in build repo due to it failing out production integTest on Jenkins. Thanks.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-988-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a844c23837830641a3273778c96179d3c5099bc4
# Push it to GitHub
git push --set-upstream origin backport/backport-988-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-988-to-2.x.

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.

Add remote cypress tests orchestrator integration as part of integtest.sh script
7 participants