fixes 'add docker-for-desktop platform to kfctl (golang)' #2433
Conversation
…nnet.go respectively
…to refactor_to_plugins
|
Marking this as WIP until #2428 is merged. |
bootstrap/Makefile
Outdated
| echo SUCCESS | ||
|
|
||
| # generate all --email john@foo.com --ipName 35.233.240.120 | ||
| test-known-platforms-generate: test-known-platforms-init | ||
| @cd ~/ksonnet && \ | ||
| kfctl generate all -V && \ | ||
| PLUGINS_ENVIRONMENT=$(GOPATH)/src/github.com/kubeflow/kubeflow/bootstrap/bin kfctl generate all -V && \ |
There was a problem hiding this comment.
I'd just do:
PLUGINS_ENVIRONMENT=$(PLUGINS_ENVIRONMENT)
bootstrap/Makefile
Outdated
| cd ~/minikube && \ | ||
| kfctl generate all -V --mount-local && \ | ||
| PLUGINS_ENVIRONMENT=$(GOPATH)/src/github.com/kubeflow/kubeflow/bootstrap/bin kfctl generate all -V --mount-local && \ |
bootstrap/Makefile
Outdated
| kfctl generate all -V --mount-local && \ | ||
| PLUGINS_ENVIRONMENT=$(GOPATH)/src/github.com/kubeflow/kubeflow/bootstrap/bin kfctl generate all -V --mount-local && \ | ||
| cd ~/docker-for-desktop && \ | ||
| PLUGINS_ENVIRONMENT=$(GOPATH)/src/github.com/kubeflow/kubeflow/bootstrap/bin kfctl generate all -V --mount-local && \ |
bootstrap/Makefile
Outdated
| kfctl generate all -V --mount-local && \ | ||
| PLUGINS_ENVIRONMENT=$(GOPATH)/src/github.com/kubeflow/kubeflow/bootstrap/bin kfctl generate all -V --mount-local && \ | ||
| cd ~/docker-for-desktop && \ | ||
| PLUGINS_ENVIRONMENT=$(GOPATH)/src/github.com/kubeflow/kubeflow/bootstrap/bin kfctl generate all -V --mount-local && \ | ||
| cd ~/foo && \ | ||
| PLUGINS_ENVIRONMENT=$(GOPATH)/src/github.com/kubeflow/kubeflow/bootstrap/bin kfctl generate all -V && \ |
bootstrap/cmd/kfctl/README.md
Outdated
| make debug | ||
| ``` | ||
|
|
||
| To go back to where plugins are loaded (this should be the default) run |
There was a problem hiding this comment.
Not sure about the wording but a better message would be a good idea
There was a problem hiding this comment.
thanks Ebi - we're going to move most of the plugins back to a static configuration to minimize complexity. The PLUGINS_ENVIRONMENT will probably be set to a specific directory when we add the brew installer.
kkasravi
force-pushed
the
dockerfordesktop
branch
from
0162a6c
to
f63dc6a
Compare
kkasravi
changed the title
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions (waiting on @ashahba and @ellis-bigelow)
bootstrap/Makefile, line 107 at r4 (raw file):
Previously, ashahba (Abolfazl Shahbazi) wrote…
I'd just do:
PLUGINS_ENVIRONMENT=$(PLUGINS_ENVIRONMENT)
thanks - i think both are updated now
bootstrap/Makefile, line 109 at r4 (raw file):
Previously, ashahba (Abolfazl Shahbazi) wrote…
same as above
Done.
bootstrap/Makefile, line 111 at r4 (raw file):
Previously, ashahba (Abolfazl Shahbazi) wrote…
same as above
Done.
bootstrap/Makefile, line 113 at r4 (raw file):
Previously, ashahba (Abolfazl Shahbazi) wrote…
same as above
since we're removing some of the plugins and making them static the Makefile doesn't need the PLUGINS_ENVIRONMENT everywhere - but thanks. There are changes from the prior PR
|
/assign @ashahba |
There was a problem hiding this comment.
Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions (waiting on @ashahba and @ellis-bigelow)
bootstrap/Makefile, line 107 at r4 (raw file):
Previously, kkasravi (Kam Kasravi) wrote…
thanks - i think both are updated now
Done.
bootstrap/Makefile, line 113 at r4 (raw file):
Previously, kkasravi (Kam Kasravi) wrote…
since we're removing some of the plugins and making them static the Makefile doesn't need the PLUGINS_ENVIRONMENT everywhere - but thanks. There are changes from the prior PR
Done.
bootstrap/cmd/kfctl/README.md, line 231 at r4 (raw file):
Previously, kkasravi (Kam Kasravi) wrote…
thanks Ebi - we're going to move most of the plugins back to a static configuration to minimize complexity. The PLUGINS_ENVIRONMENT will probably be set to a specific directory when we add the brew installer.
Done.
There was a problem hiding this comment.
Reviewed 2 of 32 files at r1, 1 of 3 files at r3, 4 of 13 files at r5.
Reviewable status: 7 of 8 files reviewed, 6 unresolved discussions (waiting on @ashahba, @ellis-bigelow, and @kkasravi)
bootstrap/pkg/client/ksonnet/ksonnet.go, line 294 at r5 (raw file):
} } name := "meta-controller-cluster-role-binding"
How come we need separate logic to delete this cluster role binding?
|
just a thought If you wanted a platform to experiment with the plugin model. docker-for-desktop might be a good one. |
There was a problem hiding this comment.
great idea, why don't i remove foo.go and move dockerfordesktop.go in it's place.
Reviewable status: 7 of 8 files reviewed, 6 unresolved discussions (waiting on @ashahba, @ellis-bigelow, and @jlewi)
bootstrap/pkg/client/ksonnet/ksonnet.go, line 294 at r5 (raw file):
Previously, jlewi (Jeremy Lewi) wrote…
How come we need separate logic to delete this cluster role binding?
it's an incomplete solution right now, I was starting to doing explicit deletes. kfctl needs to delete all cluster resources based on a list that currently is generated in the application component. We basically need to get this list from ksonnet. I'll fix under #2415.
|
@jlewi replaced foo with dockerfordesktop |
|
/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 |
) * revert to prior version * fixes kfctl (golang) rename 'ks' directory and app to ksonnet and ksonnet.go respectively * fixes 'kfctl - Fetch registry automatically' * fixes 'refactor gcp, minikube, docker-for-desktop, ack to be kfctl plugins' * change DefaultDevRepo to point to just the repo not repo/kubeflow * fixes 'refactor gcp, minikube, docker-for-desktop, ack to be kfctl plugins' * plugins for all existing KfApp instances {ksonnet, minikube, foo}. * update golang version * fixes 'docker-for-desktop' * delete meta-controller-cluster-role-binding * update README.md * remove unused var DEBUG * remove bootstrap/cmd/plugins/foo.go * merge changes from upstream master * remove old files, update README.md * removing foo, making dockerfordesktop a plugin * remove foo, make dockerfordesktop the plugin
) * revert to prior version * fixes kfctl (golang) rename 'ks' directory and app to ksonnet and ksonnet.go respectively * fixes 'kfctl - Fetch registry automatically' * fixes 'refactor gcp, minikube, docker-for-desktop, ack to be kfctl plugins' * change DefaultDevRepo to point to just the repo not repo/kubeflow * fixes 'refactor gcp, minikube, docker-for-desktop, ack to be kfctl plugins' * plugins for all existing KfApp instances {ksonnet, minikube, foo}. * update golang version * fixes 'docker-for-desktop' * delete meta-controller-cluster-role-binding * update README.md * remove unused var DEBUG * remove bootstrap/cmd/plugins/foo.go * merge changes from upstream master * remove old files, update README.md * removing foo, making dockerfordesktop a plugin * remove foo, make dockerfordesktop the plugin
fixes #2420
this PR is based off of PR #2428
This change is