Cleaning up porch API to move to v1

[ciaranjohnston]

Notes on cleaning up / removing unused types in various porch API files to move to v1:

porch/types.go

• Why do we have a top-level one? Should be versioned like the others - perhaps merge with v1alpha1/types.go
• PatchType - check to see if it is still needed - there are a bunch of different types defined (CreateFile / DeleteFile / PatchFile). Only used in the generated code
• UpstreamRef - this is a freeform text field that are using in a few places but it needs to be documented / constrained into a proper type
• ParentReference - can be removed
• Condition - why can we not use a K8S type for this rather than creating a generic one?
• Result - does not seem to be used in Porch, we will try to remove as it does not need to be published in the porch API anyway
• Lines 345 - 413 - don't look like they are used, triage to check

porch/register.go:

• Update "porch.kpt.dev" to "porch.nephio.org" - same for others
• otherwise the file seems to register the structs with the API server

porch/v1alpha1/types.go:
porchconfig/v1alpha1/types.go:

• why two separate files?
• Repository - why is this in a separate package?

controllers/packagevariants/api/v1alpha1/groupversion_info.go

And other controller APIs

api/porchinternal/v1alpha1/types.go

We will create v1alpha2 / v1alpha3 on the relevant files to make changes, then move to a "beta" when ready

Need to update all the imports to we can have a single switch to change APIs

[kispaljr]

Why do we have a top-level one? Should be versioned like the others - perhaps merge with v1alpha1/types.go

In Kubernetes apiservers there is usually an internal and multiple versions of external types for each resource. All the external types are converted to the internal type, and the actual data is stored as an internal type object. When an external type is queried the stored internal type is converted back to the queried external type.
In the porch server the porch/types.go are the internal types and the porch/v1alpha1/types.go are the external types. Keeping this structure will be useful when we will start to have multiple external type versions.

[liamfallon]

In this PR I removed the api/porch/types.go and moved the other files in api/porch into api/porch/v1alpha1, At the moment there is no difference between the structures we are using externally and internally. So all the comments on api/porch/types.go above will be addressed.

Having internal structs that we translate from and to for different versions is correct, and when we have different versions on the API we should use that approach. However, I don't think our internal structs should be in the api/porch directory, they could be in another package, maybe pkg/apitranslation or internal/pkg/apitranslation.

[kispaljr]

PatchType - check to see if it is still needed - there are a bunch of different types defined (CreateFile / DeleteFile / PatchFile). Only used in the generated code

These became obsolete with the 1-item task list change (that among others eliminates the "patch" Task) .
I think these can be removed.

[kispaljr]

UpstreamRef - this is a freeform text field that are using in a few places but it needs to be documented / constrained into a proper type

Which UpstreamRef field do you refer to? The ones that I know already have a proper go type.

[kispaljr]

ParentReference - can be removed

+1

[kispaljr]

Condition - why can we not use a K8S type for this rather than creating a generic one?

If I understand correctly, and the proposal is to use the k8s.io/apimachinery/pkg/apis/meta/v1.Condition type that is coming from the Kubernetes codebase, instead of specifying our own condition type, then I wholeheartedly agree :)

[kispaljr]

Result - does not seem to be used in Porch, we will try to remove as it does not need to be published in the porch API anyway

This is meant to be used to parse or store KRM Function execution outputs. I agree that this shouldn't be part of the porch API, but instead this should be imported from https://github.com/kptdev/kpt/tree/main/pkg/api/fnresult/v1

[kispaljr]

porch/v1alpha1/types.go:
porchconfig/v1alpha1/types.go:

why two separate files?
Repository - why is this in a separate package?

Because Repository and everything under config.porch.kpt.dev is a CRD, while everything under porch.kpt.dev is an API server-backed resource.
Probably "config" is not an ideal name for the API group for CRDs, especially if we turn PackageRevision to a CRD. We should think about finding a better name.

[liamfallon]

Could we continue the discussion over here?
EPIC: Release a hard revision of the Porch APIs

I'll consider the comments already given here on that epic issue.

[liamfallon]

Many of the comments on api/porch/types.go also apply to api/porch/v1alpha1/types.go.

I suggest we clean up the API properly and make these changes on a v1alpha2 version of the API once kptdev/porch#340 is merged. This will also allow us to bring in and check multi-version support in Porch on the API.