-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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(GH workflow): migrate periodic functional tests to GH actions #10751
feat(GH workflow): migrate periodic functional tests to GH actions #10751
Conversation
Hi @shruti2522. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
8da93d9
to
19a8d7f
Compare
Signed-off-by: shruti2522 <shruti.apc01@gmail.com> feat: migrate periodic functional tests to GH actions Signed-off-by: shruti2522 <shruti.apc01@gmail.com> feat: migrate periodic functional tests to GH actions Signed-off-by: shruti2522 <shruti.apc01@gmail.com> feat: migrate periodic functional tests to GH actions feat: migrate periodic functional tests to GH actions Signed-off-by: shruti2522 <shruti.apc01@gmail.com> feat: migrate periodic functional tests to GH actions Signed-off-by: shruti2522 <shruti.apc01@gmail.com>
2b977b0
to
cdfc9e5
Compare
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.
Thank you for your contribution! I added some comments in the workflow code, you're doing a good job!
.github/workflows/periodic.yml
Outdated
setup_kind: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Check if Docker is installed |
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.
This step can be removed. GH Actions always provide docker binary
.github/workflows/periodic.yml
Outdated
else | ||
echo "Docker is already installed." | ||
fi | ||
- name: Install KinD |
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.
GH actions have an extension to provision KinD. Check this example: https://github.com/kubeflow/pipelines/blob/master/.github/workflows/backend.yml#L36
.github/workflows/periodic.yml
Outdated
curl -Lo kind https://kind.sigs.k8s.io/dl/v0.11.1/kind-linux-amd64 | ||
chmod +x kind | ||
sudo mv kind /usr/local/bin | ||
- name: Verify KinD installation |
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.
Remove that step. Previous step if using the GH Action extension already verify installation
.github/workflows/periodic.yml
Outdated
- name: Run Functional Tests | ||
run: | ||
./test/kfp-functional-test/kfp-functional-test.sh | ||
- name: Collect test results |
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.
Although the step is present in the workflow, it requires some changes in the bash script to generate the artifacts.
Basically, what you can do is redirect this execution to a path that you create previously.
Signed-off-by: shruti2522 <shruti.apc01@gmail.com>
I have updated the code. Please take a look, @rimolive. |
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 last changes before we start testing
.github/workflows/periodic.yml
Outdated
./test/kfp-functional-test/kfp-functional-test.sh > periodic_tests.txt | ||
mkdir -p ./test/artifacts # Directory to store artifacts | ||
cp periodic_tests.txt ./test/artifacts/ | ||
|
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.
This code is important: https://github.com/kubeflow/pipelines/blob/master/.github/workflows/backend.yml#L76-L81
.github/workflows/periodic.yml
Outdated
node_image: kindest/node:v1.29.2 | ||
- name: Run Functional Tests | ||
run: | ||
./test/kfp-functional-test/kfp-functional-test.sh > periodic_tests.txt |
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 me suggest a different approach:
log_dir=$(mktemp -d)
./test/kfp-functional-test/kfp-functional-test.sh > $log_dir/periodic_tests.txt
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 mktemp
? It creates a random path under /tmp
for your temporary artifact collection task. That way, the last step only needs to get all the paths created by mktemp
, compress in a tarball, and publish to a remote path.
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've made the changes you suggested and included the artifact collection, @rimolive.
Signed-off-by: shruti2522 <shruti.apc01@gmail.com>
/ok-to-test |
.github/workflows/periodic.yml
Outdated
uses: actions/upload-artifact@v4 | ||
with: | ||
name: periodic-functional-artifacts | ||
path: $log_dir |
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.
Replace this line with the same content as in https://github.com/kubeflow/pipelines/blob/master/.github/workflows/backend.yml#L81
.github/workflows/periodic.yml
Outdated
runs-on: ubuntu-latest | ||
steps: | ||
- name: Checkout repository | ||
uses: actions/checkout@v2 |
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: upgrade to actions/checkout@v4
Signed-off-by: shruti2522 <shruti.apc01@gmail.com>
Done @rimolive |
Don't forget this. #10751 (comment) |
Signed-off-by: shruti2522 <shruti.apc01@gmail.com>
done with the changes @rimolive |
.github/workflows/periodic.yml
Outdated
@@ -19,11 +19,10 @@ jobs: | |||
node_image: kindest/node:v1.29.2 | |||
- name: Run Functional Tests | |||
run: | | |||
log_dir=$(mktemp -d) |
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.
Please, add this line back and don't make any other changes. All we need is this 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.
oh, might have done it by mistake. I will add it 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.
Done
Signed-off-by: shruti2522 <shruti.apc01@gmail.com>
/lgtm cc @chensun |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chensun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description of your changes:
Fixes #10745. I have added the workflow file and tried running it locally, but I guess there is some issue with the package versions in
requirements.txt
.Error Log:
Checklist:
CC @rimolive