nokia / danm

TelCo grade network management in a Kubernetes cluster
BSD 3-Clause "New" or "Revised" License
373 stars 81 forks source link

Update the domain for danm #269

Closed alejandrojnm closed 2 years ago

alejandrojnm commented 2 years ago

What type of PR is this?

Uncomment only one, leave it on its own line:

bug cleanup design documentation failing-test feature

What does this PR give to us: Compatibility with the new versions of Kubernetes

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

Special notes for your reviewer: I modify the client created by client-gen to add the Namespace to the Update function

Does this PR introduce a user-facing change?:

Now the new API is `danm.io/v1`, also the CRD is created using `apiextensions.k8s.io/v1` and not `apiextensions.k8s.io/v1beta1`
Levovar commented 2 years ago

1: does your commit message meant to imply you consider the comments resolved? if yes: they are not. I really fail to understand why do you keep tinkering with the existing attribute structure you must keep everything as is. literally. go through the original YAML files attribute by attribute, and retain the exat same structure, with the exact same validation rules, without modifying them, or adding anything else to the spec if for some reason this cannot be done explicitly state the reason attribute-by-attribute why modifications are required

2: so if I understand the context of client go related changes the only manual change you made here is the "Namespace" addition to the TenantConfig client's "Create" and "Update" operation if I compare those with the code the same library generates for another namespaced API, for example for DanmNet we can see these Namespace additions are there by default: https://github.com/nokia/danm/blob/master/crd/client/clientset/versioned/typed/danm/v1/danmnet.go#L115 this makes me think the client generator code is actually good, and our instructions are bad! Take a look at the following instruction set for TenantConfig: https://github.com/nokia/danm/blob/master/crd/apis/danm/v1/types.go#L116 if you compare this to the designation of DanmNet you can see the order of the instructions is different. maybe that's an issue? but more importantly if you compare the TenantConfigList instruction set to DanmNetList: https://github.com/nokia/danm/blob/master/crd/apis/danm/v1/types.go#L132 you can see we basically told the client gen library to generate the list nonNameSpaced. in all the other cases (except ClusterNetworkList) this designation is not there TL;DR let's try to 1: switch the instruction order above TenantConfig struct to match the same order used in the case of other APIs 2: remove the "nonNameSpaced" designation from TenantConfigList so it matches the other APIs then let's regenerate the client code with these changes. I hope with these corrections the generated code will be okay and we don't need to manually fix it

Levovar commented 2 years ago

@alejandrojnm looks good to me now! only thing left is to fix CI, and see if it works in a real environment

CI complains that your branch still has the manual modification in the TenantConfig client code, but with the new modifications you should be able to revert it, it should work with the baseline generated code. please test it out, and then we can merge

Levovar commented 2 years ago

@alejandrojnm looks good to me! are you still finalizing or testing it, or can I go ahead and merge?

alejandrojnm commented 2 years ago

I will test all tomorrow to see if work as expected, I will let you know, in that way you can merge

Levovar commented 2 years ago

@alejandrojnm any updates?

alejandrojnm commented 2 years ago

I will try to test this week, I have some tasks pending, sorry for the delay

alejandrojnm commented 2 years ago

Hi @Levovar we test this in kubernetes 1.24 and work, I will send all the test env when we test all, but is working 🎉