Use yaml config to manage k8s resources deployed by bootstrapper #853
Conversation
|
/assign @jlewi |
|
/assign @lluunn |
| KfVersion string | ||
| NameSpace string | ||
| Project string | ||
| ConfigDir string |
There was a problem hiding this comment.
Why a directory and not a single file?
| @@ -205,7 +249,7 @@ func setupNamespace(namespaces type_v1.NamespaceInterface, name_space string) er | |||
|
|
|||
| func createComponent(opt *options.ServerOption, kfApp *kApp.App, fs *afero.Fs, args []string) { | |||
| componentName := args[1] | |||
| componentPath := filepath.Join(opt.AppDir, "components", componentName+".jsonnet") | |||
| componentPath := filepath.Join(opt.AppDir, "Components", componentName+".jsonnet") | |||
There was a problem hiding this comment.
Why is Components capitalized?
There was a problem hiding this comment.
goland's auto rename...
| } | ||
| // Install packages. | ||
| for _, p := range appConfig.Packages { | ||
| //for _, p := range []string{"kubeflow/core", "kubeflow/tf-serving", "kubeflow/tf-job", "kubeflow/pytorch-job"} { |
bootstrap/config/gcp_prototype.yaml
Outdated
| # Sample config for kubeflow bootstrapper | ||
| --- | ||
| # Apps that always apply | ||
| DefaultApp: |
There was a problem hiding this comment.
Why do the key names start with capital letters? This doesn't follow standard K8s or GCP deployment manager syntax. Can we make it lower case?
bootstrap/config/gcp_prototype.yaml
Outdated
| @@ -0,0 +1,44 @@ | |||
| # Sample config for kubeflow bootstrapper | |||
| --- | |||
| # Apps that always apply | |||
There was a problem hiding this comment.
Why are we creating multiple apps? Why wouldn't we just create a single app and use the ConfigMap to specify the app we want to create?
| Parameters []KsParameter | ||
| } | ||
|
|
||
| type BootConfig struct { |
There was a problem hiding this comment.
Why are there multiple apps? And if you want to support multiple apps why wouldn't you use a container like a list or map.
My sense though is we should start with a single app unless we have a clear use case for multiple ones.
I think if a user wanted multiple apps they could run bootstrapper multiple times
|
Awesome; thanks for the quick fix. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlewi 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 |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/test all |
…eflow#853) * Use yaml config to manage k8s resources deployed by bootstrapper * handle review feedback
Signed-off-by: Ce Gao <gaoce@caicloud.io>
* Change Katib manifests from origin * Run tests * Remove PV from base
This provide us flexibility to control kubeflow resources when facing different code version / env.
solve #829
This change is