-
Notifications
You must be signed in to change notification settings - Fork 80
add mutating for appConfig to support name/properties format of trait #242
Conversation
Signed-off-by: 天元 <jianbo.sjb@alibaba-inc.com>
1a4fcfe
to
6b3cf46
Compare
This link is not working. Where is the design? |
@hongchaodeng Sorry for the broken link, fixed now. It's https://github.com/oam-dev/spec/blob/master/7.application_configuration.md#trait |
Object: content, | ||
} | ||
// find out the GVK from the CRD definition and set | ||
apiVersion := metav1.GroupVersion{ |
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 am OK for short term fix by simply replacing name/properties with GVK. In long term it's better to fix this in AppConfig controller so $ kubectl get appconfig
will also return name/properties 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.
Yes, this case is different from the component
mutate hook as that was completely inside the component. I preferred to handle the explicit field in the controller instead. It's actually simpler to test.
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.
component
mutate hook and this PR are actually the same thing, I hope we can use a separate PR to move them inside the controller together if we really want to do that. \cc @resouer @ryanzhang-oss
pkg/webhook/v1alpha2/applicationconfiguration/validating_handler.go
Outdated
Show resolved
Hide resolved
u: &unstructured.Unstructured{Object: map[string]interface{}{ | ||
"apiVersion": "extend.oam.dev/v1alpha2", | ||
"kind": "SimpleRolloutTrait", | ||
"metadata": map[string]interface{}{ |
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 will be good to add a name here that is different from the label so we know which is the truth
"apiVersion": "apps/v1", | ||
"kind": "Deployment", | ||
"metadata": map[string]interface{}{ | ||
"labels": map[string]interface{}{ |
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 will be good to add a name here that is different from the label so we know which is the truth
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 don't know what do you mean here? Which name?
Object: content, | ||
} | ||
// find out the GVK from the CRD definition and set | ||
apiVersion := metav1.GroupVersion{ |
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, this case is different from the component
mutate hook as that was completely inside the component. I preferred to handle the explicit field in the controller instead. It's actually simpler to test.
pkg/webhook/v1alpha2/applicationconfiguration/mutating_handler.go
Outdated
Show resolved
Hide resolved
Signed-off-by: 天元 <jianbo.sjb@alibaba-inc.com>
@ryanzhang-oss all comments are replied/fixed, please review again, thanks |
Synced off-line with @wonderflow, we realized that the mutation is not really addressing the new spec that uses the trait name to point to the |
Signed-off-by: 天元 jianbo.sjb@alibaba-inc.com