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

Add Organization type #11

Merged
merged 22 commits into from
Jan 6, 2022
Merged

Add Organization type #11

merged 22 commits into from
Jan 6, 2022

Conversation

glrf
Copy link
Contributor

@glrf glrf commented Dec 21, 2021

The Organization type is provided by a custom API server. This requires the Organization resource to be in a different group than the CRDs. We introduce the group organization.appuio.io

Summary

This PR got quite large, however a lot of the code is auto-generated or tests.

I used the apiserver-runtime to reduce the amount of boilerplate, but the core business logic is independent from the library

  • We introduce a new Organization type in apis/organization.
    • We use kubebuilder to auto-generate deepcopy but disable crd generation.
    • The type needs to be in its own group as we can only delegate whole groups to aggregate API server
    • This package also include functions to convert from and to namespaces
    • Most of the methods are there for apiserver-runtime to do its magic
  • apiserver/organization contains the actual API server implementation
    • organization.go and organization_list.go contain the organizationStorage object that implements the rest.Storage interface of Kubernetes and is the core of the api server.
    • namespaces.go is only a thin layer of abstraction to access namespaces. I introduce it to more easily mock the interaction with Kubernetes in tests (See the auto-generate mock/ package)
    • table.go handles the server side printing for kubectl

I will handle deployment and adding the api server to the local-environment as a separate PR.

The CodeClimate issues are interfaces of Kubernetes

Checklist

  • PR contains a single logical change (to build a better changelog).
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog.

@glrf glrf force-pushed the feat/organizations branch 2 times, most recently from db40c14 to 7661a74 Compare December 21, 2021 09:49
The Organization type is provided by a custom API server. This requires
the Organization resource to be in a different group than the CRDs. We
introduce the group `organization.appuio.io`
@glrf glrf force-pushed the feat/organizations branch from 7661a74 to 96cb276 Compare December 21, 2021 09:54
@glrf glrf force-pushed the feat/organizations branch from 81e127b to cf5704f Compare December 21, 2021 10:22
@glrf glrf force-pushed the feat/organizations branch from d2250f8 to 2ffadfc Compare December 22, 2021 10:51
@glrf glrf force-pushed the feat/organizations branch from ab78343 to 4e8becc Compare December 22, 2021 15:09
@glrf glrf force-pushed the feat/organizations branch from 4e8becc to ef8ae8c Compare December 22, 2021 15:17
@glrf glrf self-assigned this Dec 23, 2021
@glrf glrf force-pushed the feat/organizations branch from eb23345 to b55ca40 Compare January 4, 2022 15:09
@glrf glrf force-pushed the feat/organizations branch from b55ca40 to fff02ee Compare January 4, 2022 15:11
@glrf glrf force-pushed the feat/organizations branch from 7648f21 to da95fdf Compare January 5, 2022 10:20
@glrf glrf changed the title [POC] Add Organization type Add Organization type Jan 5, 2022
@glrf glrf added the enhancement New feature or request label Jan 5, 2022
@glrf glrf marked this pull request as ready for review January 5, 2022 14:13
@glrf glrf requested review from ccremer, simu and bastjan January 5, 2022 14:13
Copy link
Member

@simu simu left a comment

Choose a reason for hiding this comment

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

LGTM overall, I looked at the following files carefully and didn't spot any errors:

  • apis/organization/v1/organization_types.go
  • apiserver/organization/organization.go
  • apiserver/organization/organization_list.go
  • apiserver/organization/table.go

Re codeclimate: Would it make sense to tune the maintainability checks to accept the methods which currently generate warnings (cf. https://docs.codeclimate.com/docs/advanced-configuration#section-default-checks)?

tools.go Show resolved Hide resolved
apis/organization/v1/organization_types.go Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
apis/organization/v1/organization_types.go Outdated Show resolved Hide resolved
apiserver/organization/organization_list.go Outdated Show resolved Hide resolved
apiserver/organization/organization_list.go Outdated Show resolved Hide resolved
@ccremer
Copy link
Contributor

ccremer commented Jan 5, 2022

Re codeclimate: You can manually make exceptions for individual cases if you're logged in (I did it for 2 of 3). They are persistent.
not sure if you want to address the max 4 allowed return statements (I assume it's classic Go error handling 🙄 ) in this PR or just ignore for now.

@glrf
Copy link
Contributor Author

glrf commented Jan 5, 2022

Re codeclimate: You can manually make exceptions for individual cases if you're logged in (I did it for 2 of 3). They are persistent. not sure if you want to address the max 4 allowed return statements (I assume it's classic Go error handling roll_eyes ) in this PR or just ignore for now.

It has nothing to do with Go's error handling. It's just a complex interface defined by Kubernetes that we need to implement and there is nothing we can do about it.

@glrf glrf force-pushed the feat/organizations branch from 13e0fb7 to 88ad247 Compare January 5, 2022 16:51
@ccremer
Copy link
Contributor

ccremer commented Jan 5, 2022

It has nothing to do with Go's error handling. It's just a complex interface defined by Kubernetes that we need to implement and there is nothing we can do about it.

I'm pretty sure we could apply refactoring methods like "extract method" or introduce other helpers for it, but let's not make a fuss now ;)

@glrf
Copy link
Contributor Author

glrf commented Jan 5, 2022

It has nothing to do with Go's error handling. It's just a complex interface defined by Kubernetes that we need to implement and there is nothing we can do about it.

I'm pretty sure we could apply refactoring methods like "extract method" or introduce other helpers for it, but let's not make a fuss now ;)

Oh wait I completely misread the issue from codeclimate 🤦 I thought it complained about the number of return values..

So yes I guess we could "fix" that. But I'm not sure that would improve readability.

* Improve naming of helper function
* Add go generate to makefile
* Make README more copy pastable
@glrf glrf force-pushed the feat/organizations branch from a582d17 to 72d740a Compare January 5, 2022 17:25
Copy link
Contributor

@bastjan bastjan left a comment

Choose a reason for hiding this comment

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

LGTM, while I'm not super versed here I did not find any obvious code errors

🚀

apis/organization/v1/organization_types.go Outdated Show resolved Hide resolved
Co-authored-by: Chris <github.account@chrigel.net>
@glrf glrf merged commit f7008bc into master Jan 6, 2022
@glrf glrf deleted the feat/organizations branch January 6, 2022 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants