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

Refactoring of Operator #45

Closed
7 tasks done
buehler opened this issue Aug 30, 2020 · 8 comments · Fixed by #50
Closed
7 tasks done

Refactoring of Operator #45

buehler opened this issue Aug 30, 2020 · 8 comments · Fixed by #50
Labels
enhancement New feature or request released

Comments

@buehler
Copy link
Owner

buehler commented Aug 30, 2020

In collaboration with @ocdi;
Refactoring of the logic how the operator is used.

It should be more like a "normal" addon to the generic host of asp.net core and introduce it's complexity there.
Therefore we introduce some breaking changes and make a major version bump.

Topics to tackle:

  • Refactor the operator startup logic to be an extension method of the generic host
  • Return a IOperatorBuilder from services.AddKubernetesOperator that can perform more finetuning
  • Add proper code linting
  • Add proper (more or less) testing tools (maybe a WebApplicationFactory class for the operator? in context of xUnit)
  • Finetune the IOperatorBuilder methods (add resource controller, add finalizer, add ...)
  • Refactor finalizer appending -> it would be cool to have a "finalizer register" when one can add a finalizer to a resource instead of having to fetch the finalizer from DI and add a myriad of generic information to the method
  • Refactor / overlook all the generics. some types cannot be infered and it would be nice to have less generic type arguments (like the add resource controller method)

anything else? :-)

@buehler buehler added the enhancement New feature or request label Aug 30, 2020
@buehler buehler pinned this issue Aug 30, 2020
@buehler
Copy link
Owner Author

buehler commented Aug 30, 2020

Code linting is inserted to have some kind of code style #46

@ocdi
Copy link
Contributor

ocdi commented Aug 30, 2020

I started laying out IOperatorBuilder and did the obvious one AddController.

To handle finalizers I was thinking of making the AddController method look something like this, that way you can chain the finalizers and keep the context of the controller.

public OperatorBuilder AddController<T>(Action<ControllerBuilder<T>>? builder = null)
    where T : class, IResourceController
{
    _services.AddResourceController<T>();
    if (builder != null)
    {
        var cb = new ControllerBuilder<T>();
        builder(cb);
    }

    return this;
}

public class ControllerBuilder<T>
{
    public ControllerBuilder<T> Finalizer<TFin>()
    {
        return this;
    }
}

I'm not sure what to do in the Finalizer method yet though. It would be nice if we could register this as a dependency specifically to be inserted to T for the IResourceController but I don't know how to do that generically.

If we do go down this road, the ControllerBuilder should probably have a reference to the OperatorBuilder and we should expose a Services property so we can do additional DI registrations.

@buehler
Copy link
Owner Author

buehler commented Aug 31, 2020

Mhmm. Yea I know what you mean. I've got an idea about this.

@buehler
Copy link
Owner Author

buehler commented Aug 31, 2020

One big question remains on how we can add the "commands" that I added for convenience to install / uninstall crds and generate stuff?

@ocdi
Copy link
Contributor

ocdi commented Aug 31, 2020

I think most of the checklist is now done! The only possible thing I would suggest is the removal of the test code from KubeOps. I guess it probably comes down to is the test code making it easier for your unit tests in the project or for consumers of the library to make unit tests. My suggested approach would be creating a KubeOps.Testing package that can contain the test code.

What are your thoughts on that?

@buehler
Copy link
Owner Author

buehler commented Sep 1, 2020

Nice :-)
Yes I thought of that (the testing issue) when I first created the project, but back then I was more like "meh...". Now It makes absolute sense. I accidently closed your pull request when I merged my refactoring branch. I'm gonna tackle the issue right away. The goal could be to have a release today or tomorrow :-)

@buehler
Copy link
Owner Author

buehler commented Sep 1, 2020

Since the two of us have quite a difference in time between Australia and Switzerland, I took the liberty of updating the readme and splitting out the testing package myself

@github-actions
Copy link

github-actions bot commented Sep 1, 2020

🎉 This issue has been resolved in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@buehler buehler unpinned this issue Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants