refactor: Reduce tight-coupling between sync.go <-> canary.go, bluegreen.go #1461

mumoshu · 2021-08-31T03:39:27Z

Please let me start with as small as possible change :)

While I was building a PoC towards #1457, I realized that it was hard to cleanly extract the Deployer interface out of rolloutContext due to tight coupling between sync.go and canary.go + bluegreen.go.

My theory is the dependency graph should ideally be sync.go --depends-on--> bluegreen.go/canary.go --depends-on--> replicaset.go, and the codebase looks "mostly" like that.

By moving scaleReplicaSetAndRecordEvent from sync.go to replicaset.go, I'm removing the unwated reverse dependency from bluegreen.go/canary.go to sync.go, so that the codebase gets closer to the ideal state.

These source files should eventually be in dedicated sub-packages (if maintainers agree) so that the Go compiler can detect cyclic dependencies like this. Doing that in a single commit would make reviewing hard, hence I'd like to do it one by one.

If you're curious how this refactoring helps to extract the Deployer interface, see subsequent commits in my branch: master...mumoshu:refactor-replicaset-deployer

You'll see this change resulting in reducing the diff in the next commit.

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
- I take this as a (c) chore
The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
- I've provided it a refactor: prefix according to the conventional commits standard
I've signed my commits with DCO
I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
- No additional tests as this is purely a refactoring without no logic change
My builds are green. Try syncing with master if they are not.
My organization is added to USERS.md.

…een.go While I was building a PoC towards argoproj#1457, I realized that it was hard to extract the workload interface out of rolloutContext due to tight coupling between sync.go and canary.go + bluegreen.go. My theory is the dependency graph should ideally be sync.go -> bluegreen.go/canary.go -> replicaset.go. By moving `scaleReplicaSetAndRecordEvent` from sync.go to replicaset.go, I'm removing the unwated reverse dependency. These source files should eventually be in dedicated go pkgs (if maintainers agree) so that the Go compiler can detect cyclic dependencies. Doing that in a single commit would make reviewing hard, hence I'd like to do it one by one. Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>