search

kudobuilder / kudo

Kubernetes Universal Declarative Operator (KUDO)
https://kudo.dev
Apache License 2.0
1.18k stars 103 forks source link

Refactoring: Adjust api imports for new api version #1668

Closed ANeumann82 closed 4 years ago

ANeumann82 commented 4 years ago

What would you like to be added: Replace 

import "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"

with 

import kudoapi "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"

and usage of v1beta1.Instance with kudoapi.Instance

Why is this needed: Currently the code in KUDO imports the API packages like this:

import (
    "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
)

the types imported are therefore used as v1beta1.Instance, v1beta1.OperatorVersion, etc.

When we introduce a new API version v1beta2 we need to adjust all these usages, which will introduce a lot of changes in unrelated code, making it hard to review.

I propose to alias the import of a specific api to a generic "kudoapi" and only use this in the code itself. When we change the used API version, we only need to run a global replace to use the new API and the PR change will be limited to imports and actual code changes that is required because of the new API.

kensipe commented 4 years ago

I'm a fan for revving the version to v1beta1. I don't see this proposal doing anything meaningful (unless I'm missing something). what we need to decide is if we will support v1beta1 and v1beta2 concurrently... if not we bump to v1beta2 and move on. if we support both... then we need an internal model and the use of beta1 and beta2 where appropriate will likely require the aliasing of the import and it's usage in code.

The only thing I see here with the current proposal is the aliasing... IMO we shouldn't prescribe low level usage of imports... that is on an as need basis while coding... unless we are creating a standard by which we can make code generally easier to read.

I also don't think we need to proactively make import alias changes in prep for new versioned apis.

Perhaps your suggestion is to have an alias kudoapi which is swappable from one to the other... with a perceived value of swapping what the alias is pointing to without changing it's usage. This too seems off IMO... 1. we may need both 2. it would be expected that a version change has changes that happen in code as well.

I'm of the opinion lets make import and alias changes in the process of coding.

ANeumann82 commented 4 years ago

I'm a fan for revving the version to v1beta1.

I'm not sure what you mean with this sentence?

if we will support v1beta1 and v1beta2 concurrently

Depends on the definition of support :) At least the api packages needs both versions for a while for migration purposes, although the rest of KUDO will (should) use only one version at a time - Conversion will be handled completely by the conversion webhook.

then we need an internal model

There will be an internal model in any case, to comfortably convert between the different versions - this is described here pretty nicely: https://book.kubebuilder.io/multiversion-tutorial/conversion-concepts.html

There is an option to only use the internal model for KUDO code, but on the other hand the KUDO CLI and the controller receive specific versions of the resources. Using the internal model would require conversion every time on read and write. Not necessarily a blocker, but something to keep in mind.

Perhaps your suggestion is to have an alias kudoapi which is swappable from one to the other... with a perceived value of swapping what the alias is pointing to without changing it's usage. This too seems off IMO... 1. we may need both 2. it would be expected that a version change has changes that happen in code as well.

Yes - my suggestion is to have an alias kudoapi which is swappable. I wish this was possible on a gobal scope and not on a per-file base. The reason for this is that the actual change between the versions is relatively small.

For 1.: I don't think there will be a case where we need to use both in non-generated code outside of the api package, but I agree, in these cases the kudoapi alias should not be used. For 2.: A version change will need to have code changes as well. The proposed kudoapi alias simply limits the amount of unneccessary changes.

For comparison: We currently have 63 files which import the v1beta1 api and could use the kudoapi alias. These 63 files have 907 references to v1beta1. I think it makes a big difference for a review to look at 907 changes plus the actual code adjustments or 63 (one import per file) plus the actual code adjustments.

kensipe commented 4 years ago

hmm... first the description in the kubebuilder book is about APIs (and operators) and seems different. management of API versions across components with externalized (non-encapsulated) interfaces with a need to convert from one version to another and back is completely different than internal versioning IMO.

I'm not convinced but I'm not against it. I currently don't see the value.

What we need is the ability for 1 version of kudo to support potentially different versions of the API types (which conversion WH doesn't help with)... why? The CLI needs to be able to work with operator packages which are v1beta1 and v1beta2 at the same time. At some point it will need to be able to parse and manage operators from different API versions.

In order to manage this, I believe we need to have an internal model (perhaps it is versioned) and the external models (currently defined in the api package). Then we translate external versions (v1beta1 AND v1beta2) to the internal model. Much of the code which you are looking to modify today should be switched to an internal model and NOT to an external model at all... which is another way of solving the problem you are looking at.

ANeumann82 commented 4 years ago

I'm not convinced but I'm not against it. I currently don't see the value.

I've created a draft PR for this. These changes would be mixed in with actual code changes when we switch to v1beta2. I think the value of reviewing this vs a much smaller PR is quite a bit.

What we need is the ability for 1 version of kudo to support potentially different versions of the API types (which conversion WH doesn't help with)... why? The CLI needs to be able to work with operator packages which are v1beta1 and v1beta2 at the same time. At some point it will need to be able to parse and manage operators from different API versions.

There are two things here:

  1. KUDO CLI does not need to understand two CRD versions at the same time - at least not at the moment. This might be a goal for the future, but currently the CLI either talks v1beta1 or v1beta2, not both - at least I don't see any reason why it would.
  2. The KUDO package format. I agree that KUDO must be able to work with both versions here - but the package format is different from the CRDs. The structs for the the package format is in pkg/kudoctl/packages/types.go and isn't even versioned yet. The pkg/kudoctl/packages/convert package contains the code to convert from package format to a specific CRD.

In order to manage this, I believe we need to have an internal model (perhaps it is versioned) and the external models (currently defined in the api package). Then we translate external versions (v1beta1 AND v1beta2) to the internal model. Much of the code which you are looking to modify today should be switched to an internal model and NOT to an external model at all... which is another way of solving the problem you are looking at.

I've mentioned this in my previous comment, and I think using only the internal (hub) model would be a viable solution - it also solves the problem where the "helper" functions should be defined. This PR would still be helping with this switch, as it separates the tedious search-and-replace work that doesn't need much review from actual code changes.