rabbit-stalker
rabbit-stalker copied to clipboard
feat: integrate rollout
Hi Agus @craftcodedev. I was reviewing the PR and found several things to change related to following points:
1. Regarding to the order of the resources I have seen in your PR that you have put rollout in the middle of all enumeration of resources, for example, this one.
I think you did this because Rollout is more similar to a Deployment, so they should be together. At this moment we prefer to keep core resources together, and then the custom ones, alphabetical order. Moreover, as rollout is not a core resource, we would like you to specify always the vendor. Something like argoRollout ArgoRollout, etc (always respecting the case the previous code has)
// Your proposal
ResourceKindDeployment = "Deployment"
ResourceKindRollout = "Rollout"
ResourceKindStatefulSet = "StatefulSet"
ResourceKindDaemonSet = "DaemonSet"
// Our proposal
ResourceKindDaemonSet = "DaemonSet"
ResourceKindDeployment = "Deployment"
ResourceKindStatefulSet = "StatefulSet"
ResourceKindArgoRollout = "ArgoRollout"
2. Related to the moment to apply the patch against the resource in Kubernetes Regarding to this piece of code, in that function, we have separated the moment to create the patch and the moment to apply it against the actual Kubernetes resource. The idea of this separation is to create the proper patch for each resource, and then, finally, to apply it. With your code, you are applying it in the middle of the process and then, applying it again in the end, duplicating code
Hi Agus @craftcodedev. I was reviewing the PR and found several things to change related to following points:
1. Regarding to the order of the resources I have seen in your PR that you have put rollout in the middle of all enumeration of resources, for example, this one.
I think you did this because Rollout is more similar to a Deployment, so they should be together. At this moment we prefer to keep core resources together, and then the custom ones, alphabetical order. Moreover, as
rolloutis not a core resource, we would like you to specify always the vendor. Something likeargoRolloutArgoRollout, etc (always respecting the case the previous code has)// Your proposal ResourceKindDeployment = "Deployment" ResourceKindRollout = "Rollout" ResourceKindStatefulSet = "StatefulSet" ResourceKindDaemonSet = "DaemonSet" // Our proposal ResourceKindDaemonSet = "DaemonSet" ResourceKindDeployment = "Deployment" ResourceKindStatefulSet = "StatefulSet" ResourceKindArgoRollout = "ArgoRollout"2. Related to the moment to apply the patch against the resource in Kubernetes Regarding to this piece of code, in that function, we have separated the moment to create the patch and the moment to apply it against the actual Kubernetes resource. The idea of this separation is to create the proper patch for each resource, and then, finally, to apply it. With your code, you are applying it in the middle of the process and then, applying it again in the end, duplicating code
Hey Alby @achetronic,
- Done! now you will see ResourceKindArgoRollout.
- Regarding this, the reason why I split it, is because we are creating and applying the patch in a different way compared to the core resource. From my point of view the code will be difficult to understand if we decide to mix them. I can apply some design pattern (strategy) but for now I prefer to keep it simple.
Hey Agus @craftcodedev we implemented a way to include more workload managers yesterday. After a review this is merged on master.
That change make it easier for you to create a patcher for Argo Rollout. We implemented it as a map[string]*func()
This means you only need to implement a function that returns the patch you need to be applied on Argo Rollout resources, and include that function on the map. From that moment, your use case will be completely covered. Do you want us to guide you through the process?
Kudos @sebastocorp for the fantastic idea and feature development 🥳
Hello @achetronic @sebastocorp , thanks for the improvement! I will add the function to create the patch for rollouts. There are two things:
types.StrategicMergePatchTypeis not going to work for rollout. When I deployed in prod, it was failing because of it. So I changed to types.MergePatchTypeResourceKindArgoRollout = "ArgoRollout"shouldn't be ResourceKindRollout = "Rollout"?
Hey, I answered you in the issue, but the approach has changed to make it easier for users like you. So could you please open a new PR with the changes done in the way that is explained in the issue? 🙏🏼
Closing this one