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

Always send integer or boolean query parameters to the API server #160

Merged

Conversation

ityuhui
Copy link
Member

@ityuhui ityuhui commented Nov 30, 2022

Fixes #158

Regenerate this client code to merge OpenAPITools/openapi-generator#14019

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 30, 2022
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 30, 2022
@ityuhui
Copy link
Member Author

ityuhui commented Nov 30, 2022

Hi @brendandburns

This PR changes code logic to always send integer or boolean query parameters (even if its value is 0 or false) to the API server.
But sometimes (most of the time) users don't need to send this parameter to the API server, but use the default value in API server.
e.g.
When deleting a pod, if users don't speficy the gracePeriodSeconds, API server can use the default value (30 seconds if I'm not mistaken). But after this PR is merged, client library will always send a grace period value to the API server.

I suggest introducing a flag: USE_DEFAULT_VALUE_IN_API_SERVER to handle this case:

#define USE_DEFAULT_VALUE_IN_API_SERVER -9783 // a magic number
or
#define USE_DEFAULT_VALUE_IN_API_SERVER INT_MIN // INT_MIN is defined in <limits.h>

...
    if (gracePeriodSeconds != USE_DEFAULT_VALUE_IN_API_SERVER)  //  the code in this PR is `if (1)`
    {
        keyQuery_gracePeriodSeconds = strdup("gracePeriodSeconds");
        valueQuery_gracePeriodSeconds = calloc(1,MAX_NUMBER_LENGTH);
        snprintf(valueQuery_gracePeriodSeconds, MAX_NUMBER_LENGTH, "%d", gracePeriodSeconds);
        keyPairQuery_gracePeriodSeconds = keyValuePair_create(keyQuery_gracePeriodSeconds, valueQuery_gracePeriodSeconds);
        list_addElement(localVarQueryParameters,keyPairQuery_gracePeriodSeconds);
    }
...

But this solution cannot handle the type of boolean. (It's also possible, after all, boolean is actually integer in the C programming language :)

What do you think ?

@ityuhui
Copy link
Member Author

ityuhui commented Nov 30, 2022

FYI @wing328 @zhemant @michelealbano #160 (comment)

@brendandburns
Copy link
Contributor

This is a tricky case. I don't really love the idea of having a magic number to get the defaults, but I also don't love the idea that someone wouldn't get the defaults when they expect to.

Ideally we'd use int* and bool* instead of int and bool because then you can differentiate between NULL and *val == 0 but the ergonomics of passing pointers instead of values is pretty bad.

I'm ok to merge this PR as is while we figure this out though.

@brendandburns
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 1, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, ityuhui

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 [brendandburns,ityuhui]

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

@k8s-ci-robot k8s-ci-robot merged commit 3a13070 into kubernetes-client:master Dec 1, 2022
@ityuhui ityuhui deleted the yh-int-bool-para-1130 branch December 2, 2022 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot delete pod forcibly with CoreV1API_deleteNamespacedPod API
3 participants