-
Notifications
You must be signed in to change notification settings - Fork 17
CARRY: KCP example #51
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
Skipping CI for Draft Pull Request. |
a66bfe2
to
8ada4c8
Compare
8ada4c8
to
902fee2
Compare
@@ -105,7 +105,7 @@ modules: ## Runs go mod to ensure modules are up to date. | |||
|
|||
.PHONY: generate | |||
generate: $(CONTROLLER_GEN) ## Runs controller-gen for internal types for config file | |||
$(CONTROLLER_GEN) object paths="./pkg/config/v1alpha1/...;./examples/configfile/custom/v1alpha1/..." | |||
$(CONTROLLER_GEN) object paths="./pkg/config/v1alpha1/...;./examples/configfile/custom/v1alpha1/...;./examples/kcp/..." |
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.
nit: this change could also go into the example package
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.
yap, but this means we need to duplicate generators and this machinery. In my head lower price compared to keeping it here.
clusterName = logicalcluster.NewPath("root:widgets") | ||
) | ||
|
||
func main() { |
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.
am not quite sure that we need this whole bootstrapping. What do we install? Workspaces and APIExport+Schemas? Isn't that one apply?
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.
It is. But it gives a new view of how one could do this in production. I liked this pattern for bootstrapping and I think it could be useful for consumers
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.
and its in go over bash :D
@@ -0,0 +1,30 @@ | |||
apiVersion: audit.k8s.io/v1 | |||
kind: Policy |
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 this used or just from debugging?
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.
It is used in bootstrapped test servers. I think its boilerplate code from old day. Remove or leave?
examples/kcp/config/main.go
Outdated
utilruntime.Must(tenancyv1alpha1.AddToScheme(scheme)) | ||
utilruntime.Must(clientgoscheme.AddToScheme(scheme)) | ||
utilruntime.Must(corev1alpha1.AddToScheme(scheme)) | ||
utilruntime.Must(apisv1alpha1.AddToScheme(scheme)) |
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.
why a custom scheme and not just the default scheme from client-go?
examples/kcp/controllers/helper.go
Outdated
|
||
// WithClusterInContext injects a cluster name into a context such that | ||
// cluster clients and cache work out of the box. | ||
func WithClusterInContext(r reconcile.Reconciler) reconcile.Reconciler { |
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.
can we move that up into the main library?
examples/kcp/main.go
Outdated
) | ||
|
||
var ( | ||
scheme = runtime.NewScheme() |
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.
why a custom scheme? Just use the client-go scheme in client code.
examples/kcp/main.go
Outdated
utilruntime.Must(clientgoscheme.AddToScheme(scheme)) | ||
utilruntime.Must(datav1alpha1.AddToScheme(scheme)) | ||
|
||
flag.StringVar(&kubeconfigContext, "context", "", "kubeconfig context") |
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.
why is this different from the other flags?
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
/lgtm |
LGTM label has been added. Git tree hash: f17ddccd7653ed6a56a6cfdfe2bad6d48eae5241
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sttts 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 |
Backports kcp-dev/controller-runetime-example example into main repo under examples/kcp
Simplifies all the deployments to be as minimal as possible.