-
Notifications
You must be signed in to change notification settings - Fork 527
[CI][HELM] Use chart-testing to install Helm charts #3412
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
Conversation
Signed-off-by: Yi Chen <[email protected]>
879537a
to
01de5a6
Compare
Signed-off-by: Yi Chen <[email protected]>
@kevin85421 PTAL when you have time. |
cc @MortalHappiness would you mind taking a look? Thanks! |
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.
Generally LGTM. Only a small question.
- name: Build Docker image (kuberay-operator) | ||
if: steps.list-changed.outputs.changed == 'true' && matrix.chart == 'kuberay-operator' | ||
run: | | ||
cd ray-operator && make docker-image -e IMG=kuberay/operator:local | ||
|
||
- name: Build Docker image (kuberay-apiserver) | ||
if: steps.list-changed.outputs.changed == 'true' && matrix.chart == 'kuberay-apiserver' | ||
run: | | ||
cd apiserver && make docker-image -e IMG=kuberay/apiserver:local | ||
|
||
- name: Build Docker image (security-proxy) | ||
if: steps.list-changed.outputs.changed == 'true' && matrix.chart == 'kuberay-apiserver' | ||
run: | | ||
cd experimental && make docker-image -e IMG=kuberay/security-proxy:local | ||
|
||
- name: Load image to kind cluster (kuberay-operator) | ||
if: steps.list-changed.outputs.changed == 'true' && matrix.chart == 'kuberay-operator' | ||
run: | | ||
kind load docker-image kuberay/operator:local | ||
|
||
- name: Load image to kind cluster (kuberay-apiserver) | ||
if: steps.list-changed.outputs.changed == 'true' && matrix.chart == 'kuberay-apiserver' | ||
run: | | ||
kind load docker-image kuberay/apiserver:local | ||
|
||
- name: Load image to kind cluster (security-proxy) | ||
if: steps.list-changed.outputs.changed == 'true' && matrix.chart == 'kuberay-apiserver' | ||
run: | | ||
kind load docker-image kuberay/security-proxy:local |
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.
Wondering if we want to test local build images or the production image on quay.io
. This tool is used to test whether the Helm chart can be installed or not, right? So shouldn't we test the production image repo since most users won't build a KubeRay image themselves?
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.
Since this workflow runs on both master
and release-*
branch, I am afraid that we cannot use the production image for all branches. For example, if there are possible breaking changes on the master branch in the future, it may fail the workflow triggered by release branch.
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.
cc @kevin85421 Do we only want to test production images or local build images? By the way, as far as I know, the CI config for GitHub Actions is branch-specific, meaning each branch relies on the config defined in that branch. So if the config in the master branch is changed, the release branch will not be affected unless we cherry-pick the changes into it.
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.
For per-commit CI tests, we should test the local build images. Testing with the production image is a one-off test and doesn't need to run per-commit. We can add a release test to cover it. Does this make sense to you?
This reverts commit cbde878.
Why are these changes needed?
Use chart-testing (ct) install command to check whether the Helm charts can be installed successfully.
Related issue number
Close #3413
Checks