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

Workflow and playwright test updates #1360

Merged
merged 1 commit into from
Apr 29, 2024
Merged

Workflow and playwright test updates #1360

merged 1 commit into from
Apr 29, 2024

Conversation

ytimocin
Copy link
Contributor

@ytimocin ytimocin commented Apr 24, 2024

  1. Added Playwright failure videos.
  2. Removing Wait for all pods to be ready step because the previous step, rad deploy, should do the waiting already.
  3. Making sure we skip certain steps if Cancel Workflow is called.
  4. Adding the name of the EKS Cluster to the Delete EKS Cluster step. (Write EKS cluster name to issue title or description #1003)
  5. Removing rad uninstall from the Delete EKS Cluster step. (EKS cluster not deleted if rad not installed #868)
  6. Updating the dependencies in the playwright package.json file.
  7. Changing to liveness check instead of readiness check. Please see this comment.
  8. Added a logic to reload page if the catalog items are not found. Not sure if this is an eShop problem but sometimes the service that needs to return the products throws an error. Please see this video.

Fixes:
#1003
#868
#1017

HealthChecksUI__HealthChecks__10__Name: 'Ordering HTTP Background Check'
HealthChecksUI__HealthChecks__10__Uri: 'http://ordering-backgroundtasks:5111/hc'
HealthChecksUI__HealthChecks__10__Uri: 'http://ordering-backgroundtasks:5111/liveness'
Copy link
Contributor

Choose a reason for hiding this comment

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

were we using the health check incorrectly before?

Copy link
Contributor

Choose a reason for hiding this comment

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

The eshop code has a readiness check at /hc and a liveness check at /liveness. We had the readiness check hooked up as a liveness check

run: |
if [[ "${{ matrix.container }}" != "" ]]; then
rad resource expose containers ${{ matrix.container }} ${{ matrix.exposeArgs }} --port ${{ matrix.port }} &
export ENDPOINT="http://localhost:3000/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export ENDPOINT="http://localhost:3000/"
export ENDPOINT="http://localhost:${{ matrix.port }}"

@ytimocin ytimocin force-pushed the ytimocin/wtUpdates branch 3 times, most recently from 090f61b to 2f36308 Compare April 26, 2024 01:05
@@ -22,6 +22,9 @@ on:
- edge
schedule: # Run every 2 hours
- cron: "0 */2 * * *"
concurrency:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment explaining why this is needed? It's easy to forget the rationale for adding something like this later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to group workflow runs and cancel if a new workflow starts running. What I mean is that, if we create a PR, a workflow is triggered. Then if we push another commit to that PR, another workflow will be triggered and the in-progress one should be cancelled.

Now that I think about this, because we might have already created resources, I think it would be good to not cancel this workflow and make sure that the resources are deleted if created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think @rynowak?

@@ -283,14 +286,12 @@ jobs:
- name: Deploy app
if: steps.gen-id.outputs.RUN_TEST == 'true'
id: deploy-app
run: rad deploy ${{ matrix.path }} ${{ matrix.deployArgs }} -e ${{ matrix.env }}
- name: Wait for all pods to be ready
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we no longer care about waiting for the pods to be ready?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we discussed this in one of our 1:1 meeting. rad deploy should wait for all pods to be ready before returning successfully. Is that wrong?

@rynowak
Copy link
Contributor

rynowak commented Apr 26, 2024

Is this working better now that you swapped the health checks?

@ytimocin ytimocin force-pushed the ytimocin/wtUpdates branch from 2f36308 to 891c135 Compare April 26, 2024 05:51
@ytimocin
Copy link
Contributor Author

Is this working better now that you swapped the health checks?

Yes, it definitely is working better. Not passing the tests all the time but still a good improvement so far with the changes here.

@ytimocin ytimocin force-pushed the ytimocin/wtUpdates branch 2 times, most recently from 8c34b1a to 6e808c4 Compare April 26, 2024 15:43
@ytimocin ytimocin force-pushed the ytimocin/wtUpdates branch from 6e808c4 to 3c3b66c Compare April 26, 2024 21:36
@ytimocin ytimocin force-pushed the ytimocin/wtUpdates branch from 3c3b66c to cbeaf5e Compare April 27, 2024 02:39
@@ -34,21 +33,16 @@ function delete_aws_resources() {
# Empty the file
truncate -s 0 $DELETED_RESOURCES_FILE

for resource_type in ${RESOURCE_TYPES//,/ }
Copy link
Contributor

Choose a reason for hiding this comment

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

why the changes to this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just a reformat

@ytimocin ytimocin merged commit 2c0de3a into v0.33 Apr 29, 2024
8 checks passed
@ytimocin ytimocin deleted the ytimocin/wtUpdates branch April 29, 2024 20:19
sk593 pushed a commit that referenced this pull request May 23, 2024
sk593 pushed a commit that referenced this pull request May 23, 2024
sk593 added a commit that referenced this pull request May 23, 2024
* update gh to ado sync workflow to use service principals (#1363)

* Make env configurable (#1046)

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Adding test AKS workflow to v0.32 (#1052)

* Run functional tests every 2 hours and add another workflow that runs… (#1020)

* Run functional tests every 2 hours and add another workflow that runs the tests on AKS instead of k3d

Signed-off-by: ytimocin <[email protected]>

* Triggering workflow

Signed-off-by: ytimocin <[email protected]>

---------

Signed-off-by: ytimocin <[email protected]>

* Removing the run of the AKS workflow on PRs (#1051)

Signed-off-by: ytimocin <[email protected]>

* Update the timeout (30s) for Playwright

Signed-off-by: ytimocin <[email protected]>

---------

Signed-off-by: ytimocin <[email protected]>

* Use retry and update eshop playwright tests to wait for the catalog to appear (#1213)

Signed-off-by: ytimocin <[email protected]>

* update gh to ado sync workflow to use service principals

Signed-off-by: Will Tsai <[email protected]>

---------

Signed-off-by: Reshma Abdul Rahim <[email protected]>
Signed-off-by: ytimocin <[email protected]>
Signed-off-by: Will Tsai <[email protected]>
Signed-off-by: Will <[email protected]>
Co-authored-by: Reshma Abdul Rahim <[email protected]>
Co-authored-by: Yetkin Timocin <[email protected]>
Signed-off-by: sk593 <[email protected]>

* Workflow and playwright test updates (#1360)

Signed-off-by: ytimocin <[email protected]>
Signed-off-by: sk593 <[email protected]>

* Changing the place of page.reload in the tests (#1492)

Signed-off-by: ytimocin <[email protected]>
Signed-off-by: sk593 <[email protected]>

* Adding a job that will purge AWS EKS clusters every 6 hours (#1462)

Signed-off-by: ytimocin <[email protected]>
Signed-off-by: sk593 <[email protected]>

* Update all deps of all apps (#1536)

Signed-off-by: ytimocin <[email protected]>
Signed-off-by: sk593 <[email protected]>

* Updating the runner of purge azure test resources workflow (#1537)

Signed-off-by: ytimocin <[email protected]>
Signed-off-by: sk593 <[email protected]>

* Update purget test resources to fix the workflow failures (#1552)

Signed-off-by: ytimocin <[email protected]>
Signed-off-by: sk593 <[email protected]>

* Fixing the eShop catalog load issue for the UI tests (#1548)

Signed-off-by: ytimocin <[email protected]>
Signed-off-by: sk593 <[email protected]>

---------

Signed-off-by: Reshma Abdul Rahim <[email protected]>
Signed-off-by: ytimocin <[email protected]>
Signed-off-by: Will Tsai <[email protected]>
Signed-off-by: Will <[email protected]>
Signed-off-by: sk593 <[email protected]>
Co-authored-by: Will <[email protected]>
Co-authored-by: Reshma Abdul Rahim <[email protected]>
Co-authored-by: Yetkin Timocin <[email protected]>
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.

3 participants