Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor reconciler #120

Merged
merged 23 commits into from
Oct 27, 2020
Merged

Refactor reconciler #120

merged 23 commits into from
Oct 27, 2020

Conversation

Kidswiss
Copy link
Contributor

@Kidswiss Kidswiss commented Oct 21, 2020

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.

@tobru tobru changed the title Refactor reconiler Refactor reconciler Oct 21, 2020
@Kidswiss
Copy link
Contributor Author

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.

Simon Beck added 10 commits October 22, 2020 12:42
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
Copy link
Contributor Author

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.

Copy link
Contributor

@corvus-ch corvus-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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 Outdated Show resolved Hide resolved
pkg/pipeline/cluster.go Outdated Show resolved Hide resolved
pkg/pipeline/cluster_steps_test.go Outdated Show resolved Hide resolved
pkg/pipeline/cluster_steps_test.go Outdated Show resolved Hide resolved
pkg/pipeline/cluster_steps_test.go Outdated Show resolved Hide resolved
pkg/pipeline/common_steps.go Outdated Show resolved Hide resolved
pkg/pipeline/pipelines.go Outdated Show resolved Hide resolved
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
Copy link
Contributor

@corvus-ch corvus-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
pkg/pipeline/cluster_steps_test.go Outdated Show resolved Hide resolved
pkg/pipeline/cluster_steps_test.go Outdated Show resolved Hide resolved
pkg/pipeline/cluster_steps_test.go Show resolved Hide resolved
pkg/pipeline/cluster_steps_test.go Outdated Show resolved Hide resolved
pkg/pipeline/cluster_steps_test.go Outdated Show resolved Hide resolved
pkg/pipeline/cluster_steps_test.go Outdated Show resolved Hide resolved
pkg/pipeline/cluster_steps_test.go Outdated Show resolved Hide resolved
pkg/pipeline/cluster_steps_test.go Outdated Show resolved Hide resolved
pkg/pipeline/cluster_steps_test.go Outdated Show resolved Hide resolved
@Kidswiss
Copy link
Contributor Author

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.

Copy link
Contributor

@corvus-ch corvus-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
Copy link
Contributor

@corvus-ch corvus-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the work.

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

@Kidswiss
Copy link
Contributor Author

Thanks @corvus-ch

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

@Kidswiss Kidswiss merged commit e2f11a5 into master Oct 27, 2020
@Kidswiss Kidswiss deleted the reconciler branch October 27, 2020 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants