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

Making necessary changes in the running-functional-tests document. Also doing some cleanup. #8087

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ytimocin
Copy link
Contributor

Description

Making necessary changes in the running-functional-tests document. Also doing some cleanup:

  1. Removed unnecessary environment variables from the workflows.

Type of change

  • This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional).
    Fixes: #issue_number

Contributor checklist

Please verify that the PR meets the following requirements, where applicable:

  • An overview of proposed schema changes is included in a linked GitHub issue.
  • A design document PR is created in the design-notes repository, if new APIs are being introduced.
  • If applicable, design document has been reviewed and approved by Radius maintainers/approvers.
  • A PR for the samples repository is created, if existing samples are affected by the changes in this PR.
  • A PR for the documentation repository is created, if the changes in this PR affect the documentation or any user facing updates are made.
  • A PR for the recipes repository is created, if existing recipes are affected by the changes in this PR.

Copy link

github-actions bot commented Nov 25, 2024

Unit Tests

    1 files  ±0    289 suites  ±0   5m 23s ⏱️ +9s
3 536 tests +1  3 531 ✅ ±0  4 💤 ±0  1 ❌ +1 
3 535 runs  ±0  3 530 ✅  - 1  4 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 24a4e7b. ± Comparison against base commit 46dc40d.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@kachawla kachawla left a comment

Choose a reason for hiding this comment

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

Had left some comments on previous version of this PR here: #8069

@ytimocin
Copy link
Contributor Author

What's the motivation for changing file name style?

I think the naming convention for sh files is underscores, dashes, or all words together. We use camel case for now. We use both underscores and dashes.

Ref: https://google.github.io/styleguide/shellguide.html#source-filenames.

I was creating new sh files for cloud and non-cloud, so I wanted to give them new names that would also be consistent with other names in our codebase.

That being said, we need to decide if we want to use underscores or dashes because we have both. We only had one camel-case and I changed that.

@ytimocin ytimocin force-pushed the ytimocin/test-doc-update branch from 7aeb2b6 to f8a1e10 Compare November 26, 2024 18:23
@ytimocin ytimocin marked this pull request as ready for review November 26, 2024 18:23
@ytimocin ytimocin requested review from a team as code owners November 26, 2024 18:23
@radius-functional-tests
Copy link

radius-functional-tests bot commented Nov 26, 2024

Radius functional test overview

🔍 Go to test action run

Name Value
Repository radius-project/radius
Commit ref f8a1e10
Unique ID funcefc4ddb0b6
Image tag pr-funcefc4ddb0b6
Click here to see the list of tools in the current test run
  • gotestsum 1.12.0
  • KinD: v0.20.0
  • Dapr:
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.3.0
  • Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-funcefc4ddb0b6
  • Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
  • applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-funcefc4ddb0b6
  • dynamic-rp test image location: ghcr.io/radius-project/dev/dynamic-rp:pr-funcefc4ddb0b6
  • controller test image location: ghcr.io/radius-project/dev/controller:pr-funcefc4ddb0b6
  • ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcefc4ddb0b6
  • deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ corerp-cloud functional tests succeeded
✅ ucp-cloud functional tests succeeded

Comment on lines -719 to -730
# AZURE_MSSQL_RESOURCE_ID
# AZURE_MSSQL_USERNAME
# AZURE_MSSQL_PASSWORD
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 see these are not being used anymore.

@@ -729,9 +726,6 @@ jobs:
AWS_ACCESS_KEY_ID: ${{ secrets.FUNCTEST_AWS_ACCESS_KEY_ID }}
AWS_SECRET_ACCESS_KEY: ${{ secrets.FUNCTEST_AWS_SECRET_ACCESS_KEY }}
AWS_REGION: ${{ env.AWS_REGION }}
RADIUS_SAMPLES_REPO_ROOT: ${{ github.workspace }}/samples
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 only being used in the noncloud tests.

Comment on lines -350 to -347
TEMP_CERT_DIR: ${{ steps.create-local-registry.outputs.temp-cert-dir }}
SSL_CERT_FILE: ${{ steps.create-local-registry.outputs.temp-cert-dir }}/certs/${{ env.LOCAL_REGISTRY_SERVER }}/client.crt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reformat

@ytimocin ytimocin force-pushed the ytimocin/test-doc-update branch from f8a1e10 to 223c606 Compare December 3, 2024 01:45
@ytimocin ytimocin temporarily deployed to functional-tests December 3, 2024 01:45 — with GitHub Actions Inactive
@radius-functional-tests
Copy link

radius-functional-tests bot commented Dec 3, 2024

Radius functional test overview

🔍 Go to test action run

Name Value
Repository radius-project/radius
Commit ref 223c606
Unique ID funcec1695f50d
Image tag pr-funcec1695f50d
Click here to see the list of tools in the current test run
  • gotestsum 1.12.0
  • KinD: v0.20.0
  • Dapr:
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.3.0
  • Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-funcec1695f50d
  • Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
  • applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-funcec1695f50d
  • dynamic-rp test image location: ghcr.io/radius-project/dev/dynamic-rp:pr-funcec1695f50d
  • controller test image location: ghcr.io/radius-project/dev/controller:pr-funcec1695f50d
  • ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcec1695f50d
  • deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting ucp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

Copy link
Contributor

@brooke-hamilton brooke-hamilton left a comment

Choose a reason for hiding this comment

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

I was able to get all five services running with a debugger following these instructions. 🎆

A helpful note in troubleshooting would be to say that after launching the debugger, five items should be visible in the VS Code call stack panel (in the debugging tab):

  • Launch Deployment Engine
  • Launch Dynamic RP
  • Launch Applications RP
  • Launch Controller
  • Launch UCP

Initially, I had a failure because I was deploying a kind cluster using the instructions in our docs site to map ports, but those ports were conflicting with the ports the debugger was attaching to, causing 503 errors. This became apparent when I realized that the Launch Applications RP debugging configuration was failing and would disappear from the list of processes.

@@ -32,7 +32,7 @@ If you need to manually test APIs you can reach them at the following endpoints
4. Install .NET 6.0 SDK - <https://dotnet.microsoft.com/en-us/download/dotnet/6.0>.
5. Install C# VS Code extension - <https://marketplace.visualstudio.com/items?itemName=ms-dotnettools.csharp>.
6. (Optional) Configure any cloud provider credentials you want to use for developing Radius.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should explain in step 1 that the Kubernetes cluster should be created with defaults, i.e., without the port mappings required in the docs because the debugger will attempt to attach to the same ports, and they will already be used by the cluster, resulting in 503 errors when running rad commands.


This example adds a `dev` workspace:
This example adds a `dev` workspace:

```yaml
workspaces:
Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to get the same result by adding the overrides: section to the existing default workspace, instead of adding a new workspace. It works either way, but do you think that modifying the existing workspace is a simpler way to understand it? In other words, override the ucp property of the current connection instead of creating a new workspace, like this. (I intend this comment as a discussion rather than a strong opinion on it.)

workspaces:
    default: default
    items:
        default:
            connection:
                context: kind-kind
                kind: kubernetes
                overrides:
                    ucp: http://localhost:9000
            environment: /planes/radius/local/resourceGroups/default/providers/Applications.Core/environments/default
            scope: /planes/radius/local/resourceGroups/default

### Run
### Run Non-Cloud Functional Tests

1. Required environment variables:
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are the values for these found so that they can be set?

Copy link

codecov bot commented Jan 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.07%. Comparing base (710509a) to head (a617d63).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8087      +/-   ##
==========================================
- Coverage   60.09%   60.07%   -0.02%     
==========================================
  Files         584      584              
  Lines       38675    38675              
==========================================
- Hits        23240    23234       -6     
- Misses      13729    13732       +3     
- Partials     1706     1709       +3     

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

@@ -86,16 +86,16 @@ env:
# Private Git repository where terraform module for testing is stored.
TF_RECIPE_PRIVATE_GIT_SOURCE: "git::https://github.com/radius-project/terraform-private-modules//kubernetes-redis"
# bicep-types ACR url for uploading Radius Bicep types
BICEP_TYPES_REGISTRY: 'biceptypes.azurecr.io'
BICEP_TYPES_REGISTRY: "biceptypes.azurecr.io"
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 give some context on why is this update needed?

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 a refactoring of the functional-test-cloud.yaml file. Not an extension but built-in feature ("shift + opt + F") in VS Code cleans it up.

@@ -373,8 +373,8 @@ jobs:
chmod +x ./bicep
sudo mv ./bicep /usr/local/bin/bicep
bicep --version

- name: Publish bicep types

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these tabs/spaces automatically being updated by an extension you are using?

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 a refactoring of the functional-test-cloud.yaml file. Not an extension but built-in feature ("shift + opt + F") in VS Code cleans it up.

@@ -364,7 +362,6 @@ jobs:
RADIUS_SAMPLES_REPO_ROOT: ${{ github.workspace }}/samples
BICEP_RECIPE_REGISTRY: "${{ env.LOCAL_REGISTRY_NAME }}:${{ env.LOCAL_REGISTRY_PORT }}"
BICEP_RECIPE_TAG_VERSION: ${{ env.BICEP_RECIPE_TAG_VERSION }}
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this gets set?

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 yaml is for non-cloud tests. If you see the references of GH_TOKEN, you can see that it is only being used in a cloud test.

Comment on lines +30 to +47
rad group create default
rad env create kind-radius
rad group create kind-radius

# Check if TF_RECIPE_MODULE_SERVER_URL environment variable is set
if [ -z "$TF_RECIPE_MODULE_SERVER_URL" ]; then
echo "Error: TF_RECIPE_MODULE_SERVER_URL environment variable is not set."
exit 1
fi

# Check if DOCKER_REGISTRY environment variable is set
if [ -z "$DOCKER_REGISTRY" ]; then
echo "Error: DOCKER_REGISTRY environment variable is not set."
exit 1
fi

# Check if BICEP_RECIPE_REGISTRY environment variable is set
if [ -z "$BICEP_RECIPE_REGISTRY" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remind where is this handled currently?

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 not being handled anywhere currently in the helper script.

Copy link
Contributor

Choose a reason for hiding this comment

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

How's everyone running functional tests today? Is this documented somewhere for manual setup?

Comment on lines +19 to +23
# Make sure that you have default environment in the default group.
# The way to check that is to run the following commands:
# `rad group switch default`
# `rad env list` and check if there is an environment named `default`.
# If not, create one using `rad env create default`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This script seems to be creating group and env below L30-L32, so why is this manual check needed?

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 being extra defensive.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems unnecessary here.

# `rad env list` and check if there is an environment named `default`.
# If not, create one using `rad env create default`.

# To run functional tests, you have to be in `kind-radius` group and
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you share context on why this is the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we have some hard-coded environment names in our functional tests: https://github.com/search?q=repo%3Aradius-project%2Fradius+%22kind-radius%22&type=code.

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 update the comment to include the context?

Comment on lines +30 to +32
rad group create default
rad env create kind-radius
rad group create kind-radius
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it need to create two groups?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The link you referenced points to all occurrences of kind-radius - can you help me understand how does that result into requiring two groups to be created?

@@ -30,12 +30,12 @@ Then you can access the recipe download at:
http://localhost:8999/<recipe_name>.zip
```

----
---
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this update do?

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 a refactoring of the README.md. That extra - was unnecessary and "shift + opt + F" in VS Code cleans it up.

@ytimocin ytimocin force-pushed the ytimocin/test-doc-update branch from a617d63 to 24a4e7b Compare January 2, 2025 23:34
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