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

Cypress - version update from 9.5.4 to 12.8.1 #598

Conversation

jakubp-eliatra
Copy link

@jakubp-eliatra jakubp-eliatra commented Mar 24, 2023

Description

Cypress version update from 9.5.4. to 12.8.1, which introduces official support for cross-origin testing.

Issues Resolved

Resolves the issue of navigating between IdP and OSD in OIDC test, also should resolve the issue of upgrading to v11. :)

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.

@jakubp-eliatra jakubp-eliatra requested a review from a team as a code owner March 24, 2023 09:27
@jakubp-eliatra jakubp-eliatra marked this pull request as draft March 24, 2023 09:28
@jakubp-eliatra jakubp-eliatra changed the base branch from main to 2.x March 24, 2023 09:30
@jakubp-eliatra jakubp-eliatra changed the base branch from 2.x to main March 24, 2023 09:33
@jakubp-eliatra jakubp-eliatra marked this pull request as ready for review March 24, 2023 09:34
@jakubp-eliatra jakubp-eliatra changed the title cypress automatic v12 migration Cypress - version update to 12.8.1 Mar 24, 2023
@jakubp-eliatra jakubp-eliatra changed the title Cypress - version update to 12.8.1 Cypress - version update from 9.5.4 to 12.8.1 Mar 24, 2023
@jakubp-eliatra jakubp-eliatra marked this pull request as draft March 24, 2023 12:27
@jakubp-eliatra jakubp-eliatra changed the base branch from main to 2.x March 24, 2023 12:34
@jakubp-eliatra jakubp-eliatra changed the base branch from 2.x to main March 24, 2023 12:34
Signed-off-by: Jakub Przybylski <jakub.przybylski@eliatra.com>
@jakubp-eliatra jakubp-eliatra marked this pull request as ready for review March 24, 2023 12:44
@cwperks
Copy link
Member

cwperks commented Mar 24, 2023

Thank you @jakubp-eliatra ! @tianleh @CCongWang @ohltyler @ashwin-pc @kavilla Could a maintainer please approve the CI to be run for this PR?

security-dashboards-plugin requires an upgrade of Cypress to Cypress 12 which includes support for cross-origin testing which is needed for functional tests involving external IdPs. When an External IdP is used as the authentication provider of OpenSearch, the security-dashboards-plugin redirects to the login screen of the identity provider (different origin) and is redirected to OSD on successful login. In order to add functional tests for these use-cases in this repo, the version of Cypress needs to be upgraded. We currently have selenium-based functional tests in the security-dashboards-plugin repo, but would like to move them here.

See companion PR that introduces functional tests for OIDC: #595

Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

Hi,

Thanks @jakubp-eliatra for the PR and @cwperks for the explanation on the purpose of this upgrade.

However, the changes are very big and the images/AMI for all integTest needs to be regenrate, and it is a very heavy process needs coordination.

Please give your thought on this: @opensearch-project/opensearch-dashboards-test

As of now to prevent the merge breaking everything, as then current agent/runner does not have cypress12 installed yet, I will need to block this PR for a bit.

Thanks.

@peterzhuamazon
Copy link
Member

Have offline discussion with @cwperks this is a major change that needs to be synced with build repo and ci repo.
All corresponding jenkinsfile also need changes.

Propose these steps:

  1. Maintainers review the PR and have a call or whether this upgrade is ok to merge
  2. Open an issue in build repo and tag me as assignee, describe the status of the PR and the ask.
  3. I will raise PRs to upgrade corresponding images/amis/jenkisnfiles and merge/generate the new versions.

We can then route back to this PR and start merging.

Thanks.

@peterzhuamazon
Copy link
Member

We need to make sure the test can be run on both x64 and arm64 architectures.
Run on LINUX/macOS/Windows platforms on the compatibility.
Thanks.

@peternied
Copy link
Member

I will raise PRs to upgrade corresponding images/amis/jenkisnfiles and merge/generate the new versions.

@peterzhuamazon Thanks for looking into this. Food for thought, there was a similar scenario for build manifest specifying the docker instance name and there was an arg parameter that allowed the jdk version to be set. When doing this upgrade if we can we source the setup/configuration from details in this repository, or move the modified elements to the test manifest future changes would be easier.

@peterzhuamazon
Copy link
Member

peterzhuamazon commented Apr 5, 2023

I will raise PRs to upgrade corresponding images/amis/jenkisnfiles and merge/generate the new versions.

@peterzhuamazon Thanks for looking into this. Food for thought, there was a similar scenario for build manifest specifying the docker instance name and there was an arg parameter that allowed the jdk version to be set. When doing this upgrade if we can we source the setup/configuration from details in this repository, or move the modified elements to the test manifest future changes would be easier.

Hi,

OSD is completely different from OS and that requires a much bigger overhaul.

As for cypress it is locked on the ftrepo instead of build repo as it is not related.
I am even thinking about removing the jdk lock in args for docker as it should be handled in OS core.

The docker arg was an earlier measure to just allow us to hardcode a version.
I think it now shows the limitation over time.

Thanks.

@cwperks
Copy link
Member

cwperks commented May 8, 2023

@peterzhuamazon What's required to unblock this PR? Are there a list of issues somewhere that need to be complete? Happy to lend a hand to move this along so that cypress can be upgraded.

@peterzhuamazon
Copy link
Member

@peterzhuamazon What's required to unblock this PR? Are there a list of issues somewhere that need to be complete? Happy to lend a hand to move this along so that cypress can be upgraded.

Hi @cwperks

Just need this PR to be sorted out with the conflict files, then I will queue up the image changes in both the images and AMIs before merging this in.

Bare in mind we are currently having some issues upgrading Jenkins, so the process will take some weeks, but we can start by getting the conflicting files sorted out.

Thanks.

@cwperks
Copy link
Member

cwperks commented May 9, 2023

@jakubp-eliatra Can you update this branch and resolve the conflicts with the main branch?

@cwperks
Copy link
Member

cwperks commented May 23, 2023

@jakubp-eliatra Can you update this branch and resolve conflicts?

@peterzhuamazon
Copy link
Member

Since there is no more activities here will replace with this one:

Thanks.

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.

4 participants