🌱 Update contract package to match documentation

[Jont828]

What this PR does / why we need it: As described in #5309, we're going through internal/contract package and updating it to match the specifications in the docs.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #7119 #7120

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[Jont828]

@killianmuldoon @fabriziopandini

[killianmuldoon]

Looks like a good start - could you add some unit tests to show these fields working?

[sbueringer]

/ok-to-test

I would take a look once the tests are green / linter is happy

[Jont828]

@sbueringer @killianmuldoon I held off on tests since I had a couple design considerations I wanted to ask about.

1. I noticed we're created an Int64 type to represent in Int32 fields, was that an intentional change to group them all as Int64?
2. I think it would be helpful to documents optional fields as well, but currently there isn't a good way to show the difference between an optional field like failureMessage and a required field like ready.
3. At what point do we want to create additional structs for nested fields? For example, InfrastructureClusterContract returns a field InfrastructureClusterControlPlaneEndpoint which has additional fields so that it would be called as infra.ControlPlaneEndpoint().Host(). Do we want every nested path to have a intermediate struct like infra.Status().Ready() or just have it be infra.Ready()?

[sbueringer]

@sbueringer @killianmuldoon I held off on tests since I had a couple design considerations I wanted to ask about.

1. I noticed we're created an Int64 type to represent in Int32 fields, was that an intentional change to group them all as Int64?
2. I think it would be helpful to documents optional fields as well, but currently there isn't a good way to show the difference between an optional field like failureMessage and a required field like ready.
3. At what point do we want to create additional structs for nested fields? For example, InfrastructureClusterContract returns a field InfrastructureClusterControlPlaneEndpoint which has additional fields so that it would be called as infra.ControlPlaneEndpoint().Host(). Do we want every nested path to have a intermediate struct like infra.Status().Ready() or just have it be infra.Ready()?

• 1 I think this is due to:
  • The contract (control-plane.md) just specifices that e.g. replicas is an int. Not if it's int32 or int64. I think int64 is just safer
  • The unstructured package only has a func for int64: unstructured.NestedInt64
• 2 I think it's fine to have the same funcs as in the usual cases and if the field is not set we just return an error that wraps errNotFound like we do today in other funcs (e.g. .status.replicas is also not always set)
• 3 In general we want the full path and everything except the leaf should be a struct

[Jont828]

Gotcha that makes sense to me. I also spoke with @jackfrancis and we were also wondering if there's a way to generate code for this as opposed to manually implementing the paths and structs.

[Jont828]

@sbueringer I've worked on implementing the optional types but had some trouble dealing with types that were a list or a map of additional structs. Would definitely appreciate any suggestions on how to proceed!

[sbueringer]

I'll take a look early next week

[sbueringer]

Gotcha that makes sense to me. I also spoke with @jackfrancis and we were also wondering if there's a way to generate code for this as opposed to manually implementing the paths and structs.

Somehow forgot to answer. I think it might be possible but I wonder if maintaining the generation code (+ unit tests, ..) is worth it. It's probably also complex code that won't be easy to understand.

[fabriziopandini]

Made a first pass, this is a great work of research.
I'm looking forward to get this merged and start cleaning up usage of unstructured all around the codebase

[Jont828]

@fabriziopandini @sbueringer I squashed and fixed the linting issues, PTAL when you get the chance!

[Jont828]

@fabriziopandini @sbueringer I think this should be ready for a final pass!

[sbueringer]

Thx!

I'll take a look as soon as I can, but I can't promise it will happen before KubeCon. Just too much things going on right now.

[fabriziopandini]

I'm +1 to merge this code, so we can start using it as a replacement for current unstructured handling across the code base.
/lgtm

[Jont828]

@fabriziopandini @sbueringer Are we good to merge this or do we need a final pass of reviews?

[sbueringer]

Looks good to me!

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sbueringer]

@Jont828 I edited the PR description so that this PR doesn't close #5309.

While this PR extends the contract package it doesn't refactor controllers to use the contract package (yet).