-
Notifications
You must be signed in to change notification settings - Fork 830
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
Support helm-less install #466
Conversation
Signed-off-by: cwen0 <cwenyin0@gmail.com>
Signed-off-by: cwen0 <cwenyin0@gmail.com>
Signed-off-by: cwen0 <cwenyin0@gmail.com>
Signed-off-by: cwen0 <cwenyin0@gmail.com>
install.sh
Outdated
OPTIONS: | ||
-v, --version Version of chaos-mesh, default value: latest | ||
-l, --local [kind] Choose a way to run a local kubernetes cluster, supported value: kind, | ||
If this value is not set and the Kubernetes is not installed, this script will exit with 1. | ||
-n, --name Name of Kubernetes cluster, default value: kind | ||
-c --crd The URL of the crd files, default value: https://raw.githubusercontent.com/pingcap/chaos-mesh/master/manifests/crd.yaml | ||
-r --runc Runtime specifies which container runtime to use. Currently we only supports docker and containerd. default value: docker |
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.
What about --runtime
, runc
is not that appropriate here
OPTIONS: | ||
-v, --version Version of chaos-mesh, default value: latest | ||
-l, --local [kind] Choose a way to run a local kubernetes cluster, supported value: kind, | ||
If this value is not set and the Kubernetes is not installed, this script will exit with 1. | ||
-n, --name Name of Kubernetes cluster, default value: kind | ||
-c --crd The URL of the crd files, default value: https://raw.githubusercontent.com/pingcap/chaos-mesh/master/manifests/crd.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.
What is the use case that we need to expose this flag? Does it need to be configurable?
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.
Using a relative path is enough 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.
If we use a relative path
, we must use exec this script in specific directory.
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.
Yes, but we have to. It is not appropriate to use /master
in such script. If we want to run this script in the release branch, then what is the value of crd
? Do we have to update to https://raw.githubusercontent.com/pingcap/chaos-mesh/<tag>/manifests/crd.yaml
every time?
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.
When we need to release, we also need to change the image to install, so I think we also need to update this, or we add a flag to define the version of chaos-mesh.
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 still think it is better to use relative path
. If using master
, then how to test each PR in CI? We have to update the path to each PR's CRD?
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 pr has no relation to CI. Why need to use this pr in 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.
Sorry I mean not CI, just in each branch. If we are in different branches and run this script, then we have to still update the crd url every time? I don't think it is reasonable
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.
Maybe we can support to set a relative path
and use the URL as a default value.
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
CA_BUNDLE=$(openssl base64 -A -in ${tmpdir}/ca.crt) | ||
|
||
cat <<EOF | ||
--- |
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.
Is it possible to not copy all these yaml templates in this script? It would be a pain that we have to update both this script and the helm templates when there is a change.
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
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 just want to provide a more convenient installation method and do not rely on helm, Do you have any other good solutions?
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.
Maybe we can add a hack script to help use us update 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.
Maybe a hack script can be executed after make yaml
and checked by 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.
Done
- --runtime | ||
- ${runc} | ||
- --http-port | ||
- !!str 31766 |
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.
!!str
is not needed
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.
If we delete !!str
, the program will report an error.
Error from server (BadRequest): error when creating "STDIN": DaemonSet in version "v1" cannot be handled as a DaemonSet: v1.DaemonSet.Spec: v1.DaemonSetSpec.Template: v1.PodTemplateSpec.Spec: v1.PodSpec.Containers: []v1.Container: v1.Container.Command: []string: ReadString: expects " or n, but found 3, error found in #10 byte of ...|tp-port",31766,"--gr|..., bigger context ...|s-daemon","--runtime","containerd","--http-port",31766,"--grpc-port",31767],"image":"pingcap/chaos-d|...
- --http-port | ||
- !!str 31766 | ||
- --grpc-port | ||
- !!str 31767 |
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.
!!str
is not needed
Signed-off-by: cwen0 <cwenyin0@gmail.com>
Signed-off-by: cwen0 <cwenyin0@gmail.com>
@Gallardot @yeya24 @fewdan PTAL again! 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.
LGTM
who and when need to execute the ‘make update-install-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.
Great work! LGTM
when we update helm charts, we need run |
/merge |
Your auto merge job has been accepted, waiting for:
|
/run-all-tests |
@cwen0 merge failed. |
Signed-off-by: cwen0 cwenyin0@gmail.com
What problem does this PR solve?
#431
What is changed and how does it work?
Check List
Tests