search

kubernetes-client / c

Official C client library for Kubernetes
Apache License 2.0
141 stars 45 forks source link

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

Closed ityuhui closed 1 year ago

ityuhui commented 1 year ago

Fixes https://github.com/kubernetes-client/c/issues/158

Regenerate this client code to merge https://github.com/OpenAPITools/openapi-generator/pull/14019

ityuhui commented 1 year ago

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 commented 1 year ago

FYI @wing328 @zhemant @michelealbano https://github.com/kubernetes-client/c/pull/160#issuecomment-1331611270

brendandburns commented 1 year ago

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 commented 1 year ago

/lgtm /approve

k8s-ci-robot commented 1 year ago

[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](https://github.com/kubernetes-client/c/blob/master/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