Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set empty objects instead error on create #44

Closed
wants to merge 1 commit into from

Conversation

rawmind0
Copy link

@rawmind0 rawmind0 commented May 6, 2021

Issues rancher/rancher#32440 and rancher/rancher#32553

This PR contains a proposal to fix commented issues. The gke-operator is not distinguishing between nil objects and empty objects. For not required field, it should generate empty objects instead of returning an error if they are nil. This is affecting the use of rancher go cli as explained at related issues.

I've built a Rancher server using this patched gke-operator and the tests seems working fine using go cli and ui for imported and new gke clusters

@cmurphy cmurphy requested a review from rmweir May 6, 2021 16:10
@cmurphy
Copy link
Contributor

cmurphy commented May 6, 2021

@rmweir can you give your thoughts on this? Since this is strictly for creates I think it could work.

@cmurphy
Copy link
Contributor

cmurphy commented May 6, 2021

It doesn't address the core issue which is how norman handles empty fields

@rawmind0
Copy link
Author

rawmind0 commented May 6, 2021

It doesn't address the core issue which is how norman handles empty fields

Agreed, Norman should generate same types at API and go cli. This is just proposed as alternative

@rmweir
Copy link

rmweir commented May 6, 2021

I'm not in favor of this fix. We should avoid writing to the spec, if we can as it should be driven by the user. We should take a deeper look at norman since I don't think this needs to be done for v2.5.8.

@rmweir
Copy link

rmweir commented May 6, 2021

An additional issue is that I don't think our integration implementation in rancher expects gke-operator to change the spec and would possible overwrite it unless we change code there.

@rmweir rmweir removed their request for review May 7, 2021 18:02
@cmurphy
Copy link
Contributor

cmurphy commented Jun 8, 2021

We're going to move toward annotating the nullable fields with rancher/norman#400 to generate them as pointers in the client code, that way clients will be able to differentiate between nil and empty values and this won't be needed. Thank you!

@cmurphy cmurphy closed this Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants