search

argoproj-labs / argo-rollouts-manager

Kubernetes Operator for Argo Rollouts controller.
https://argo-rollouts-manager.readthedocs.io/en/latest/
Apache License 2.0
100 stars 329 forks source link

Confusing operand name and resource references in codebase #1

Closed jaideepr97 closed 10 months ago

jaideepr97 commented 1 year ago

The new operator creates a Custom resource with kind "argorollout" which is the representation for the rollouts-controller created on the cluster in response to this resource. However, the resource watched by this controller is also called a "rollout". As a result, users of this operator would see 2 resources, an "argorollout" which is actually for the rollout-controller, and then a "rollout" which is the actual rollout itself, and this can be very confusing for users

Suggested solution:

I would also suggest extending this renaming to parts of the operator code, such as function names, their associated comments, log messages etc. This would make it more clear for people working on this project and make it harder to get confused about whether this operator is acting on the rollouts-controller resources or resources for the rollouts themselves
For example:

iam-veeramalla commented 1 year ago

Nice suggestion, Makes total sense :). As a coincidence I got the same feedback from Leo and Zack(Maintainers of Argo Rollouts).

A small change to your suggestion of the name, I think rollouts-manager will sound more appropriate custom resource name.

iam-veeramalla commented 1 year ago

Regarding the below points 

    `reconccileRolloutsControllerService` vs `reconcileRolloutsService` (and associated comments and log messages)
    `reconcileRolloutsControllerRole` vs `reconcileRolloutsRole`

I don't think we should change the names of these methods as they are correct. We are actually reconciling the Rollouts Role and not Rollouts Controller Role.

jaideepr97 commented 1 year ago

Regarding the below points

reconccileRolloutsControllerService vs reconcileRolloutsService (and associated comments and log messages) reconcileRolloutsControllerRole vs reconcileRolloutsRole

I don't think we should change the names of these methods as they are correct. We are actually reconciling the Rollouts Role and not Rollouts Controller Role.

The purpose of this role is to reconcile rollouts, but rollouts is a resource though, it wouldn't have a serviceaccount of its own, right? I just meant that the role is associated to the controller's serviceaccount so we should call the function that

jgwest commented 10 months ago

Read through the description and comments, and it appears all issues have been addressed.