-
Notifications
You must be signed in to change notification settings - Fork 886
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
adds ambassador and argo components to manifests #1
Conversation
/assign @kkasravi |
Issue for reference/paper trail: kubeflow/kubeflow#2566 |
@@ -0,0 +1,2 @@ | |||
bases: | |||
- ../../base |
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.
Do we need to define an overlay if the overlay isn't doing any customization?
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 thought I'd get an initial commit in and add the overlays specific to each platform later
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 wouldn't expect most packages to require platform specific customization.
So I would only expect us to define overlays for those packages and platforms requiring customization.
But this PR seems to be implying that we will define overlays for every platform for every package because you are creating stubs to be filled out later.
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.
Oh yeah, that makes sense. I'll remove stubs for components that don't require customization.
@@ -0,0 +1,5 @@ | |||
bases: | |||
- ambassador/base |
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.
Shouldn't there be a kustomization file for each package so that each application can be installed independently?
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, there is one for each component.
This is to combine everything and deploy the set of components.
I've explained the reasoning here: #4
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 each platform we will have different packages that we want to install; e.g. there are GCP specific packages.
We also have different configurations for each platform;
e.g.
GCP + IAP
GCP + Basic Auth
Minikube
For each of the above there is a different set of applications to install and possibly different overlays to be used for each application.
Is the intent for the root kustomization.yaml to support all of that? Or is the root kustomization.yaml only going to provide a vanilla K8s cconfig.
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.
Yeah, the root customization can choose what components to install from the different components.
So for minikube/gke, we'd have an overlay based on the root customization.
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.
Cool.
How did you generate these manifests? Did you just do ks show? |
Yes, ks show |
I think this is good to go once you remove the empty overlays. |
Sorry I didn't see your comment, will update the PR. Thanks for the review ! |
/lgtm This is great thanks for the quick work. |
[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 |
/cc @kkasravi @jlewi
I'm starting with ambassador and argo because I already have the converted components. I'll add the remaining components pretty soon.
This change is