Refactor reconciler #120

Kidswiss · 2020-10-21T08:30:00Z

The reconcile functions are now splitted into smaller logical step functions that can be composed.

These step functions all follow the same interface, so they can be composed at will and accept an interface type for the CRD. There are a few exceptions that will cast the interfaces to concrete objects within the function.

Also each controller now strictly only manipulates its own objects. The git repository objects are still created by other controllers, but changes won't be propagated. Instead the git repositories will now collect the information by themselves.

Relates to: #80

Checklist

Keep pull requests small so they can be easily reviewed.
Update the documentation.
Update the ./CHANGELOG.md.
Link this PR to related issues.

pkg/controller/cluster/cluster_controller.go

Kidswiss · 2020-10-21T14:05:51Z

handler.EnqueueRequestsFromMapFunc helps quite a lot with the issues I had. Now the gitRepositories can watch tenants and clusters for changes and immediately apply the changed files to the repositories.

Right now I'm adding at least some simple unit tests to all the steps that are now single functions. After that the changes should be okay to review.

Add tenant as owner to clusters

With this change the pull methods now work instantly. If a `cluster` is created, it will trigger a `tenant` reconcile via owner reference. The tenant reconcile in turn triggers a gitrepo reconcile via the `EnqueueRequestsFromMapFunc` and watches on both `clusters` and `tenants`.

Some changes to the order of thins make the unit tests fail. Also there were tests that checked the older push method for gitRepo changes. Those were removed.

Kidswiss · 2020-10-22T10:51:17Z

Finished all the new unittests and fixes for the old ones.

This can now be reviewed. Not sure if I should split this up in some smaller PRs though.

corvus-ch

Keep an eye on the scope of assertions
Consider to use loops for the pipelines
Consider to use maps for test cases

pkg/helpers/values.go

pkg/pipeline/cluster.go

pkg/pipeline/cluster_steps_test.go

pkg/pipeline/common_steps.go

pkg/pipeline/pipelines.go

pkg/pipeline/common_steps_test.go

pkg/controller/gitrepo/gitrepo_controller.go

pkg/pipeline/common.go

pkg/pipeline/common_steps.go

corvus-ch

All those excessive empty lines makes the code visually appear restless. This is irritating me. I have left some suggestions but then stopped as it started to become excessive.

I suggest to at least drop empty lists after { and before }.

pkg/pipeline/cluster.go

pkg/pipeline/cluster_steps_test.go

Kidswiss · 2020-10-23T14:25:05Z

I took almost all afternoon to find some very weird and racy issues with the integration tests. After some time I found that our implementation with loops to apply the steps was deeply flawed when using maps. They are not ordered in Go. So the order of the steps was random each time.

I changed it to a slice and struct now everything seems to be working properly again.

corvus-ch

The code style of one test file completely goes against what was established within the others. Please align that one with the others.

pkg/pipeline/git_test.go

pkg/pipeline/pipelines.go

They are no longer necessary.

corvus-ch

LGTM. Thanks for the work.

Disclaimer. I only reviewed coding style and coding structure. I am unable to assert functionality of the code.

Kidswiss · 2020-10-27T10:38:56Z

Thanks @corvus-ch

From the automated reconcile tests and my manual testing the functionality should be okay.

tobru changed the title ~~Refactor reconiler~~ Refactor reconciler Oct 21, 2020

srueg reviewed Oct 21, 2020

View reviewed changes

pkg/controller/cluster/cluster_controller.go Show resolved Hide resolved

Simon Beck added 10 commits October 22, 2020 12:42

Refactored everything

40a1621

Fix race conditions

d9645b8

Add tenant as owner to clusters

Cherry pick changes since refactor

8c22648

Update gitignore

dd58cbb

Fix tenant repo cleanup

ac9f0de

Also move unit tests

69c22f4

Add lots of unit tests

508cf82

Last batch of unit tests

371bc69

Fix failing unit tests

39de878

Some changes to the order of thins make the unit tests fail. Also there were tests that checked the older push method for gitRepo changes. Those were removed.

Kidswiss force-pushed the reconciler branch from 976880a to 39de878 Compare October 22, 2020 10:46

Update changelog

53bc48e

Kidswiss requested review from srueg, corvus-ch, simu and chrisglass October 22, 2020 10:51

corvus-ch suggested changes Oct 22, 2020

View reviewed changes

srueg reviewed Oct 22, 2020

View reviewed changes

pkg/pipeline/common_steps_test.go Outdated Show resolved Hide resolved

Remove kubectl dependency

9764494

srueg reviewed Oct 23, 2020

View reviewed changes

pkg/controller/gitrepo/gitrepo_controller.go Outdated Show resolved Hide resolved

pkg/pipeline/common.go Outdated Show resolved Hide resolved

pkg/pipeline/common_steps.go Show resolved Hide resolved

corvus-ch suggested changes Oct 23, 2020

View reviewed changes

Simon Beck added 4 commits October 23, 2020 16:03

Use loops to execute steps

3dca636

Get rid of generic utils package

d6afaee

Move test cases outside of tests funcs

efdeb37

Fix assert scopes

5d54680

Kidswiss force-pushed the reconciler branch from e1b1f85 to 074335f Compare October 23, 2020 14:15

Simon Beck added 5 commits October 26, 2020 08:11

Implement PR suggestions

6405e5e

Trim excessive whitespaces

4e0bd2d

Remove ineffective assignement

17c9781

Remove unused code

a6d311c

Remove unused code

19fcb0f

Kidswiss force-pushed the reconciler branch from 6e1bcbb to 19fcb0f Compare October 26, 2020 07:11

Kidswiss requested review from corvus-ch and srueg October 26, 2020 09:54

srueg approved these changes Oct 26, 2020

View reviewed changes

corvus-ch suggested changes Oct 27, 2020

View reviewed changes

pkg/pipeline/git_test.go Outdated Show resolved Hide resolved

pkg/pipeline/git_test.go Outdated Show resolved Hide resolved

pkg/pipeline/git_test.go Outdated Show resolved Hide resolved

pkg/pipeline/pipelines.go Outdated Show resolved Hide resolved

Simon Beck added 2 commits October 27, 2020 10:28

Align git repo unit tests with the rest

5ccb0cf

Remove resultNotOK and wrapError

8586189

They are no longer necessary.

Kidswiss requested a review from corvus-ch October 27, 2020 09:33

corvus-ch approved these changes Oct 27, 2020

View reviewed changes

Kidswiss merged commit e2f11a5 into master Oct 27, 2020

Kidswiss deleted the reconciler branch October 27, 2020 10:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor reconciler #120

Refactor reconciler #120

Kidswiss commented Oct 21, 2020 •

edited

Loading

Kidswiss commented Oct 21, 2020

Kidswiss commented Oct 22, 2020

corvus-ch left a comment

corvus-ch left a comment

Kidswiss commented Oct 23, 2020

corvus-ch left a comment

corvus-ch left a comment

Kidswiss commented Oct 27, 2020

Refactor reconciler #120

Refactor reconciler #120

Conversation

Kidswiss commented Oct 21, 2020 • edited Loading

Checklist

Kidswiss commented Oct 21, 2020

Kidswiss commented Oct 22, 2020

corvus-ch left a comment

Choose a reason for hiding this comment

corvus-ch left a comment

Choose a reason for hiding this comment

Kidswiss commented Oct 23, 2020

corvus-ch left a comment

Choose a reason for hiding this comment

corvus-ch left a comment

Choose a reason for hiding this comment

Kidswiss commented Oct 27, 2020

Kidswiss commented Oct 21, 2020 •

edited

Loading