-
Notifications
You must be signed in to change notification settings - Fork 14
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
Adding test workflow #52
Adding test workflow #52
Conversation
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
APP_NAME: local-dev-recipe-app | ||
APP_NAMESPACE: local-dev-recipe-app | ||
jobs: | ||
test: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is not supposed to run on forks please add the if check like this: https://github.com/radius-project/radius/blob/main/.github/workflows/assets.yaml#L29.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this check be added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we should run this on fork as well, wondering if there is reason why we dont want to run on forks?
.github/workflows/test.yaml
Outdated
name: local_dev_recipe_test_container_logs-pod-logs | ||
path: recipe-tests/pod-logs/local_dev_recipe_test_container_logs | ||
retention-days: 30 | ||
if-no-files-found: error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new line and reformat file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may need a reformat and a new line here
local-dev/rabbitmqqueues.bicep
Outdated
|
||
@description('Memory limit for the rabbitmq deployment') | ||
param memoryLimit string = '1024Mi' | ||
param memoryLimit string = '2048Mi' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we need to increase the limits? Increasing the memory request should be fine in my opinion. Do we need to add any limits and/or requests for the CPU?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was testing something while testing the recipe, there is no need to increase the limits. I will revert these changes back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were they reverted?
local-dev/rabbitmqqueues.bicep
Outdated
queue: queue | ||
queue: 'queue' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this pointing to an object before and now why is it a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it was pointing to string as well. I corrected it here to the right queue name
local-dev/rabbitmqqueues.bicep
Outdated
@@ -17,17 +17,14 @@ limitations under the License. | |||
@description('Information about what resource is calling this Recipe. Generated by Radius. For more information visit https://docs.radapp.dev/operations/custom-recipes/') | |||
param context object | |||
|
|||
@description('Name of the queue. Defaults to the name of the Radius RabbitMQ resource.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being removed? Usually we default these to be the name of the resource but allow them to be overridden
local-dev/sqldatabases.bicep
Outdated
@@ -75,7 +65,7 @@ resource sql 'apps/Deployment@v1' = { | |||
{ | |||
// This container is the running sql instance. | |||
name: 'sql' | |||
image: 'mcr.microsoft.com/azure-sql-edge:${tag}' | |||
image: 'mcr.microsoft.com/mssql/server:2022-latest' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we changing the container image as part of this PR? Did azure sql edge not work as expected?
tests/test-local-dev-recipes.bicep
Outdated
mongoazure: { | ||
templateKind: 'bicep' | ||
plainHTTP: true | ||
templatePath: 'reciperegistry:5000/recipes/local-dev/mongodatabases:latest' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make reciperegistry:5000
a parameter with a default value so it could be overridden if needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And could we make latest
a parameter as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
tests/test-local-dev-recipes.bicep
Outdated
container: { | ||
image: magpieimage | ||
env: { | ||
CONNECTION_SQL_CONNECTIONSTRING: db.connectionString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being set manually?
.github/workflows/test.yaml
Outdated
id: wait-for-pods | ||
run: | | ||
label="radapp.io/application=${APP_NAME}" | ||
kubectl wait --for=condition=Ready pod -l $label -n ${APP_NAMESPACE} --timeout=5m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this where we wait for the tests to complete? Does the magpie container not move to ready until all its tests are complete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right magpie container does move to ready until all tests are complete, I just added one more step to check all pods are running fine after readiness check passed.
RAD_CLI_URL=https://raw.githubusercontent.com/radius-project/radius/main/deploy/install.sh | ||
RAD_CLI_EDGE_URL=ghcr.io/radius-project/rad/linux-amd64:latest | ||
|
||
if [[ $VERSION == "edge" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still have edge version accessible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this install script from samples https://github.com/radius-project/samples/blob/d611db9ee7687bf643b09570f979a0eeb99f4e25/.github/scripts/install-radius.sh#L23-L24
APP_NAME: local-dev-recipe-app | ||
APP_NAMESPACE: local-dev-recipe-app | ||
jobs: | ||
test: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this check be added?
.github/workflows/test.yaml
Outdated
- name: Install Dapr | ||
run: | | ||
helm repo add dapr https://dapr.github.io/helm-charts/ | ||
helm install dapr dapr/dapr --version=1.6 --namespace dapr-system --create-namespace --wait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should try to use more recent Dapr if that doesn't break anything.
.github/workflows/test.yaml
Outdated
id: wait-for-pods | ||
run: | | ||
label="radapp.io/application=${APP_NAME}" | ||
kubectl wait --for=condition=Ready pod -l $label -n ${APP_NAMESPACE} --timeout=5m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@willdavsmith found another command that basically waits for the deployment to finish. You may want to check that one. It should in one of the merged PRs in the samples repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated it
.github/workflows/test.yaml
Outdated
name: local_dev_recipe_test_container_logs-pod-logs | ||
path: recipe-tests/pod-logs/local_dev_recipe_test_container_logs | ||
retention-days: 30 | ||
if-no-files-found: error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may need a reformat and a new line here
local-dev/rabbitmqqueues.bicep
Outdated
|
||
@description('Memory limit for the rabbitmq deployment') | ||
param memoryLimit string = '1024Mi' | ||
param memoryLimit string = '2048Mi' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were they reverted?
local-dev/rabbitmqqueues.bicep
Outdated
@@ -17,17 +17,14 @@ limitations under the License. | |||
@description('Information about what resource is calling this Recipe. Generated by Radius. For more information visit https://docs.radapp.dev/operations/custom-recipes/') | |||
param context object | |||
|
|||
@description('Name of the queue. Defaults to the name of the Radius RabbitMQ resource.') | |||
param queue string = context.resource.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we remove this?
local-dev/sqldatabases.bicep
Outdated
@@ -17,9 +17,6 @@ limitations under the License. | |||
@description('Information about what resource is calling this Recipe. Generated by Radius. For more information visit https://docs.radapp.dev/operations/custom-recipes/') | |||
param context object | |||
|
|||
@description('Name of the SQL database. Defaults to the name of the Radius SQL resource.') | |||
param database string = context.resource.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why this was also removed?
local-dev/sqldatabases.bicep
Outdated
@@ -75,7 +65,7 @@ resource sql 'apps/Deployment@v1' = { | |||
{ | |||
// This container is the running sql instance. | |||
name: 'sql' | |||
image: 'mcr.microsoft.com/azure-sql-edge:${tag}' | |||
image: 'mcr.microsoft.com/mssql/server:2022-latest' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2022-latest is not an image that will change, right? Not like latest
?
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I think @AaronCrawfis should also take a look.
local-dev/secretstores.bicep
Outdated
@@ -47,5 +47,6 @@ output result object = { | |||
type: daprType | |||
version: daprVersion | |||
metadata: daprComponent.spec.metadata | |||
componentName: daprComponent.metadata.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required? And if so should we add it to all the other dapr recipes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its not required, removed it
tests/test-local-dev-recipes.bicep
Outdated
param magpieimage string | ||
|
||
param registry string | ||
|
||
param version string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add descriptions to these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added descriptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple small things but overall looks great!
Signed-off-by: Vishwanath Hiremath <[email protected]>
@vishwahiremat hi i was tryna do a local deployment but having trouble with the local registry, would you help me figure what exactly its complaining about?
What i need help with, its tryna fetch the manifest, how do i make sure that available ? here is the full error if it helps
|
Hi @SantaHub , |
From the error message, it looks like the issue is the hostname |
Added a new workflow test.yaml:
Addresses radius-project/radius#5775