-
Notifications
You must be signed in to change notification settings - Fork 316
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
Document how to CRD. Add helm copier. #406
Conversation
@@ -7,7 +7,6 @@ metadata: | |||
name: mutating-webhook-configuration | |||
webhooks: | |||
- clientConfig: | |||
caBundle: Cg== |
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.
Removed by 0.4.1
: kubernetes-sigs/controller-tools#495
@@ -43,7 +43,8 @@ spec: | |||
properties: | |||
config: | |||
description: Config is an arbitrary map of configuration values used by Connect proxies. Any values that your proxy allows can be configured globally here. Supports JSON config values. See https://www.consul.io/docs/connect/proxies/envoy#configuration-formatting | |||
type: Any | |||
format: byte |
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.
The new version of the controller-tools changes this. Any
was a bug actually: kubernetes-sigs/controller-tools#505. It was never supported. Our CRD in helm use object
. I've modified the hack script to swap this when copying over to consul-helm
.
because that messes up the CRD code generation. | ||
|
||
You should be able to follow the other "normal" types. The non-normal types | ||
are `ServiceIntention` and `ProxyDefault` because they have special behaviour |
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.
behaviour
Thank you for correctly spelling this :D 🏴
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 should probably use the American spelling for consistency with ya'll 😄
@@ -0,0 +1,87 @@ | |||
// Script to move generated CRD yaml into consul-helm and modify it to match | |||
// the expected consul-helm format. |
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.
just a thought but it might be work mentioning somewhere in here that the binary (appears to be) expects to be run from CWD and not from top level consul-helm? Maybe I'm not reading the code correctly
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 thanks for calling this out. I think the better pattern is to call this through the Makefile
. I've pushed a new commit with that change. Then you can run make ctrl-crd-copy helm=../consul-helm
and it will work.
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.
Ah fantastic idea, love it!
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.
Looks awesome, great work, I cant wait to follow it the next time I'm building a new resource!
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 is such a helpful change, thanks for taking the time to document all this!
|
||
Spec IngressGatewaySpec `json:"spec,omitempty"` | ||
- Status IngressGatewayStatus `json:"status,omitempty"` | ||
+ Status `json:"status,omitempty"` |
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.
Would this be metav1.Status?
+ Status `json:"status,omitempty"` | |
+ metav1.Status `json:"status,omitempty"` |
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.
So this is actually our own Status object defined here: https://github.com/hashicorp/consul-k8s/blob/master/api/v1alpha1/status.go#L71
- Add how to create CRDs to contributing.md. - Add a script that will copy new CRDs to consul-helm in the format expected. This will reduce manual copying errors.
controller-runtime 0.4.1 fixed the issue where they generated types as Any which was an invalid type (kubernetes-sigs/controller-tools#505) however that results in the type for proxy-defaults.config being "byte" which fails when creating proxy-defaults. I played around a long time trying to find a type that generates the CRD as expected and can be unmarshalled as expected and nothing worked so for now I think it's best to keep it as json.RawMessage and then patch the generated CRD.
ACL replication changelog, remove federation.enabled
expected. This will reduce manual copying errors.
I've tested this by running through the readme to create ingress and terminating gateway CRDs and the script was used for hashicorp/consul-helm#714
Reviewers should review the script and suggest improvements to the instructions.