danm icon indicating copy to clipboard operation
danm copied to clipboard

Update the domain for danm

Open alejandrojnm opened this issue 2 years ago • 6 comments

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`

alejandrojnm avatar May 03 '22 20:05 alejandrojnm

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 avatar May 12 '22 20:05 Levovar

@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 avatar May 31 '22 19:05 Levovar

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

Levovar avatar Jun 06 '22 17:06 Levovar

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

alejandrojnm avatar Jun 06 '22 17:06 alejandrojnm

@alejandrojnm any updates?

Levovar avatar Jun 16 '22 20:06 Levovar

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

alejandrojnm avatar Jun 20 '22 09:06 alejandrojnm

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 🎉

alejandrojnm avatar Sep 15 '22 13:09 alejandrojnm