Change integration test sysytem from KinD Cluster to Minikube Cluster #1899
Conversation
| @@ -2,6 +2,9 @@ name: E2E Test with darts-cnn-cifar10 | |||
| on: | |||
| - pull_request | |||
|
|
|||
| env: | |||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |||
There was a problem hiding this comment.
Why do we need GITHUB_TOKEN? I don't think, it is exposed to pull_request type for forked repos anways.
There was a problem hiding this comment.
@johnugeorge Thanks for your review.
According to this document, it seems that actions-setup-minikube calls GitHub REST API.
So, I added GITHUB_TOKEN.
Would you like to remove GITHUB_TOKEN?
There was a problem hiding this comment.
https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token Token has only read permission and hence it would be ok. I am thinking if there will be any security concern
There was a problem hiding this comment.
For unauthenticated requests, it is limited to 60 per hour https://docs.github.com/en/rest/overview/resources-in-the-rest-api#requests-from-personal-accounts Hence, we will need GitHub token if we use this action.
There was a problem hiding this comment.
Yes, it makes sense.
I don't have any concerns about the security issues of GITHUB_TOKEN. Do you have any concerns?
| - name: Set Up KinD Cluster | ||
| uses: helm/kind-action@v1.2.0 | ||
| - name: Set Up Minikube Cluster | ||
| uses: manusa/actions-setup-minikube@v2.6.0 |
There was a problem hiding this comment.
What are the pros and cons of using this action vs manual steps(like in Kserve)?
There was a problem hiding this comment.
I used this action to change to Minikube Cluster with minimal changes.
Do you prefer this action or a manual installation?
If you prefer manual installation rather than this action, I will change the way to install Minkube.
There was a problem hiding this comment.
Can we keep the manual way as kserve? Do you see any red flags?
There was a problem hiding this comment.
There is no difference in operation between this action and manual installation.
The only difference is whether we manage the scripts or the administrator of this action manages the scripts.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johnugeorge, tenzen-y 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 |
|
Thanks for the quick review! @johnugeorge |
What this PR does / why we need it:
As discussed in our community meeting, we need to change the integration test system from KinD Cluster to Minikube Cluster to be able to use RWX PVC.
/assign @johnugeorge @andreyvelich
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Checklist: