-
Notifications
You must be signed in to change notification settings - Fork 43
Conversation
/assign @chuckha |
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.
yay organization 📁
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amy, chuckha 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 |
Not directly related to this change, but any reason to share the same package between cluster and machine actuators? @chuckha |
@detiber Mostly this is because I prefer fewer packages. If they were their own package, they would each only contain one file and one type, which is somewhat of a go antipattern. Since the implementation is fairly small and we are limited to the Cluster actuator and the Machine actuator I see little benefit of keeping each in its own package. What benefits do you see us gaining from having separate packages? |
Mainly naming, That said, I have no strong objections to keeping them in the same package. |
A fair point! I don't really think about types in packages as having to share code though. It's more of an organizational thing for me 🤔. The naming could use improving for sure though. Anyway, this is totally out of scope to the PR, sorry @amy:) |
/lgtm |
I moved the Cluster actuator related code from
actuators.go
tocluster.go
under/actuators
. This copies the pattern used foractuators/machine.go