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

feat: Validate individual tasks in GA CI #1447

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions .github/workflows/run-task-tests.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
name: Run Task Tests

"on":
pull_request

jobs:
run-task-tests:
runs-on: ubuntu-22.04
steps:
- name: Get all changed files in the PR from task or test directories
id: changed-files
uses: tj-actions/changed-files@v45
with:
# Avoid using single or double quotes for multiline patterns
files: |
task/**
test/**

- name: Free Disk Space
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

took it from here, don't aware of the exact reason, but I think might be helping to create more disk space in the github action env. Don't find any issue to keep it our CI as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

How long does this take to run? According to the README, around 3 minutes https://github.com/jlumbroso/free-disk-space?tab=readme-ov-file
Which doesn't seem worth it unless really 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.

Removed it now. Please check here.

if: steps.changed-files.outputs.any_changed == 'true'
uses: jlumbroso/free-disk-space@main
with:
tool-cache: false
docker-images: false

- name: Checkout build-defintions Repository
if: steps.changed-files.outputs.any_changed == 'true'
uses: actions/checkout@v3
with:
ref: "${{ github.event.pull_request.head.sha }}"
path: build-definitions

- name: Checkout konflux-ci/konflux-ci Repository
if: steps.changed-files.outputs.any_changed == 'true'
uses: actions/checkout@v3
with:
repository: 'konflux-ci/konflux-ci'
path: konflux-ci
Comment on lines +33 to +38
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we pin konflux-ci/konflux-ci to a commit (and configure renovate to update it)? To avoid CI breaking overnight due to changes in the repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to use specific commit from konflux-ci/konflux-ci while checking out. But for configuring the renovate to update the SHA, does only renovate config is sufficient or do we need to run any script on push at konflux-ci/konflux-ci repo?

Copy link
Contributor

@chmeliik chmeliik Nov 18, 2024

Choose a reason for hiding this comment

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

should be doable with a Renovate regex manager, roughly similar to https://docs.renovatebot.com/modules/manager/regex/#advanced-capture

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed it in the new PR

ref: f6d9d0fe8f34199eb118febcbf7f7944ae7772a9

- name: Create k8s Kind Cluster
if: steps.changed-files.outputs.any_changed == 'true'
uses: helm/kind-action@v1
with:
config: konflux-ci/kind-config.yaml

- name: Show version information
if: steps.changed-files.outputs.any_changed == 'true'
run: |
kubectl version
kind version

- name: Deploying Dependencies
if: steps.changed-files.outputs.any_changed == 'true'
run: |
cd $GITHUB_WORKSPACE/konflux-ci
./deploy-deps.sh

- name: Wait for the dependencies to be ready
if: steps.changed-files.outputs.any_changed == 'true'
run: |
cd $GITHUB_WORKSPACE/konflux-ci
./wait-for-all.sh

- name: Deploying Konflux
if: steps.changed-files.outputs.any_changed == 'true'
run: |
cd $GITHUB_WORKSPACE/konflux-ci
./deploy-konflux.sh

- name: List namespaces
if: steps.changed-files.outputs.any_changed == 'true'
run: |
kubectl get namespace

- name: Deploy test resources
if: steps.changed-files.outputs.any_changed == 'true'
run: |
cd $GITHUB_WORKSPACE/konflux-ci
./deploy-test-resources.sh

- name: Run the task tests
if: steps.changed-files.outputs.any_changed == 'true'
env:
CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }}
run: |
echo "Files changed in PR: ${CHANGED_FILES}"
cd $GITHUB_WORKSPACE/build-definitions
./test/test-tasks.sh
53 changes: 53 additions & 0 deletions .github/workflows/task-lint.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
---
name: tasklint
"on":
pull_request:
branches: [main]
jobs:
tasklint:
runs-on: ubuntu-latest
steps:
- name: Get all changed files in the PR from task directories
id: changed-files
uses: tj-actions/changed-files@v45
with:
# Avoid using single or double quotes for multiline patterns
files: |
task/**

- name: Setup Go
if: steps.changed-files.outputs.any_changed == 'true'
uses: actions/setup-go@v5
with:
go-version: '1.22'

- name: Install tektor
if: steps.changed-files.outputs.any_changed == 'true'
run: |
go install github.com/lcarva/tektor@4e9385885cc638fe337bcb9cc5f3d64cc8c262c5

- name: Run tektor linter
if: steps.changed-files.outputs.any_changed == 'true'
env:
CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }}
run: |
#!/bin/bash
echo "Files changed in PR: ${CHANGED_FILES}"
function print_changed_tasks_to_console() {
echo ${CHANGED_FILES} | grep -o 'task/[^\/]*/[^\/]*/*[^/]*.yaml' || true
}
function validate_task_yaml_using_tektor() {
all_tasks="$*"
for task in ${all_tasks}; do
# Skip if it is kustomize related yaml files
if [[ ${task} == *"kustomization.yaml" || ${task} == *"patch.yaml" || ${task} == *"recipe.yaml" ]]; then
continue
fi
tektor validate ${task}
done
}
# Validate task yamls using tektor linter
tasks_changed=$(print_changed_tasks_to_console)
[[ ! -z ${tasks_changed} ]] && {
validate_task_yaml_using_tektor "${tasks_changed}"
}
27 changes: 27 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,33 @@ Specify the Quay repository using the `QUAY_NAMESPACE` environment variable in t
./hack/test-shellspec.sh`
```

## Testing Tasks

Tasks can be validated in the GithubActions CI setup with this repository.

If you have a directory called `tests/` inside the task directory, CI will create a random `Namespace`, apply the task first in the namespace and then all the test yaml files (containing TaskRun or PipelineRun) inside the `tests` directory.

We can use the functionality of running scripts before applying the tested task or the other yaml files. There are two different scripts that are automatically applied if present. These are applied using the `source` bash script:

- `pre-apply-task-hook.sh`: Script to run before applying the task, it can include `kubectl` commands to apply any kubernetes resources in the cluster
- `pre-apply-taskrun-hook.sh`: Script to run before applying the task run or pipeline run yaml files, it can also include `kubectl` commands to apply any kubernetes resources in the cluster

For example refer: Git Clone task [tests](./task/git-clone/0.1/tests/run.yaml)

CI also validates the task yamls using [tektor](https://github.com/lcarva/tektor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@lcarva is there any plans to move it to some public org from personal account?

Copy link
Contributor

Choose a reason for hiding this comment

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


### Running Task tests locally

Prerequisites: Deploy [upstream konflux](https://github.com/konflux-ci/konflux-ci?tab=readme-ov-file#bootstrapping-the-cluster) locally.

A helper script called `run-test.sh` is present inside `test` directory to help the developer run the test. Just specify resource type as first argument, resource name as second argument and the version as third argument i.e:

```
./test/run-test.sh task git-clone 0.1
Comment on lines +154 to +157
Copy link
Contributor

Choose a reason for hiding this comment

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

type as first argument, resource name as second argument and the version as third argument

This seems unnecessary, passing the actual directory as the argument would work much better with tab completion :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could also test stepactions in similar ways, that functionality is not added fully in the current PR, which we can extend to in the future, so kept the type (task or stepactions) as first argument in the run-test.sh script.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would ./test/run-test.sh stepactions/eaas-copy-secrets-to-ephemeral-cluster/0.1 not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that should work.... updated it now

```

and it will use your current kubernetes context to run the test and show you the outputs similar to the CI.

### Compliance

Task definitions must comply with the [Enterprise Contract](https://enterprisecontract.dev/) policies.
Expand Down
4 changes: 4 additions & 0 deletions task/git-clone/0.1/tests/pre-apply-task-hook.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#!/bin/bash

echo "example of pre-apply-task-hook, this script can be used to run kubernetes commands"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should have such sample stuff in a real task, even an older version of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since we had only one example test for now, I think this might help people to understand how the pre-apply-task-hook.sh script might look like, it is kind of placeholder for now, I guess we can remove it in future when we have some tests which will contain pre-apply-task-hook.sh. WDYT?

${KUBECTL_CMD} get ns
3 changes: 3 additions & 0 deletions task/git-clone/0.1/tests/pre-apply-taskrun-hook.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#!/bin/bash

echo "an example of pre-apply-taskrun-hook script"
30 changes: 30 additions & 0 deletions task/git-clone/0.1/tests/run.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considering using a Pipeline to encapsulate the Task execution and the required verification? The release folks took this approach, example: https://github.com/konflux-ci/release-service-catalog/blob/development/tasks/check-data-keys/tests/test-check-data-keys-fail-missing-cdn-key.yaml

Copy link
Contributor Author

@tisutisu tisutisu Nov 15, 2024

Choose a reason for hiding this comment

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

@lcarva could you please help with more context around where these tests are running what approach they are following... any doc or CI pipeline script?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here we should verify that the task really cloned the repo as expected (e.g. contains the expected files, is really at revision 1.0.0, ...). And this test should set the example for how to verify that the task works as expected.

That will probably require wrapping the Task in a Pipeline as Lui suggested

Copy link
Contributor Author

@tisutisu tisutisu Nov 19, 2024

Choose a reason for hiding this comment

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

makes sense now, this is another approach to validate the tasks (we were not aware of this while investigating), the approach taken release-service-catalog repo has more flexibility on what to verify in the task results. With current approach taken in this PR, we are just validating task results are non-empty. If we want to choose the approach taken in release-service-catalog repo, the current PR needs to be rewritten almost.

Copy link
Member

Choose a reason for hiding this comment

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

As a contributor to the release-service-catalog, I just want to say that I like the approach they're using. Once I got the hang of writing mocks.sh for my tests there, it became easy to add new functional tests for my tasks including both positive and negative cases.

They have nice features, like the ability to assert that the task should fail hard and exit with a non-zero status code on certain inputs, or to assert that the task should succeed, but have certain properties in its output.

For local development / inner-loop, I can stand up kind locally, install upstream tekton, and then run export TEST_ITEMS=path/to/the/task/Im/working/on and then run the .github/scripts/test_tekton_tasks.sh from their repo in a loop while I hack on my task or test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ralphbean thanks for the feedback, looks like the approach used in release-service-catalog is better, will move to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

took this approach in this PR and closed the current one

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
name: git-clone-run-noargs
spec:
workspaces:
- name: output
emptyDir: {}
taskRef:
name: git-clone
params:
- name: url
value: https://github.com/kelseyhightower/nocode
---
apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
name: git-clone-run-tag
spec:
workspaces:
- name: output
emptyDir: {}
taskRef:
name: git-clone
params:
- name: url
value: https://github.com/kelseyhightower/nocode
- name: revision
value: 1.0.0
Loading
Loading