-
Notifications
You must be signed in to change notification settings - Fork 128
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
base: main
Are you sure you want to change the base?
Conversation
17aa0a1
to
0cd56c4
Compare
923122a
to
59f79e3
Compare
ccafb0b
to
f93dad2
Compare
@@ -0,0 +1,30 @@ | |||
--- |
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.
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
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.
@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?
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.
Can we check task results / logs / created images, etc. ?
task/** | ||
test/** | ||
|
||
- name: Free Disk Space |
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 do we need this?
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.
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.
|
||
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) |
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.
@lcarva is there any plans to move it to some public org from personal account?
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.
hack/run-test.sh
Outdated
|
||
tns=${NAME}-${VERSION//./-} | ||
|
||
# Delete the tns if already exists |
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'd expand tns
here
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 now.
hack/run-test.sh
Outdated
tns=${NAME}-${VERSION//./-} | ||
|
||
# Delete the tns if already exists | ||
${KUBECTL_CMD} delete ns ${tns} >/dev/null 2>/dev/null || : |
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.
namespace
is used in other places, please use it here too
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 now.
hack/run-test.sh
Outdated
$KUBECTL_CMD create sa appstudio-pipeline -n ${tns} | ||
fi | ||
|
||
test_resource_creation ${RESOURCE}/${NAME}/${VERSION}/tests |
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.
nitpick: missing new line
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.
some left overs, forgot to remove this file, it got moved to test/run-test.sh
local reason='' | ||
local maxloop=60 # 10 minutes max | ||
|
||
set +x |
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 do we need set +x
here?
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.
to avoid unnecessary logs being printed in the console, while we looping through to check task run status, reason and results, if something fails show_failure()
will dump the test resources in the console.
test/common.sh
Outdated
done | ||
} | ||
|
||
function test_resource_creation() { |
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.
The function name and what id does differs
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 to test_resource_creation_and_validation
, hope the name fits with its functionality now
test/common.sh
Outdated
echo ${CHANGED_FILES} |grep -o 'task/[^\/]*/[^\/]*/*[^/]*.yaml'|xargs -I {} dirname {}|awk '{print $1"/tests"}' | ||
} | ||
|
||
function get_new_changed_tasks() { |
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.
function name is not clear
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.
changed it to print_changed_tasks_to_console
, hope it better fit its functionality.
test/common.sh
Outdated
echo ${CHANGED_FILES} |grep "test/" || true | ||
} | ||
|
||
function detect_new_changed_resources() { |
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.
function name is unclear
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.
changed it to print_changed_resources_to_console
now
@@ -0,0 +1,47 @@ | |||
#!/bin/bash |
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.
Isn't it duplicates hack/run-test.sh
?
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.
forgot to remove the old one hack/run-test.sh
, this test/run-test.sh
is the correct one.
- 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 |
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 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
- name: Install tektor | ||
if: steps.changed-files.outputs.any_changed == 'true' | ||
run: | | ||
go install github.com/lcarva/tektor@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.
Similar question here, perhaps pin to a commit?
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.
Pinned to a sha now
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 |
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.
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 :)
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 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.
test/common.sh
Outdated
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 | ||
} |
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.
Let's not mix linting and tests, this should be a separate GH workflow IMO
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.
agree, will separate them to two different workflows.
This PR adds a Github Actions CI to validate the task tests (if present inside the task directory) and few example tests for
git-clone
task for reference.Story: https://issues.redhat.com/browse/STONEBLD-2910