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

Add the examples to demo listing/creating pods #3

Merged
merged 3 commits into from
Mar 20, 2020

Conversation

ityuhui
Copy link
Member

@ityuhui ityuhui commented Mar 19, 2020

Add two examples to demo how to use the c client library:

  • Create a pod
  • List pods

Because now kuberentes-client/c has no utility such as "config" functions in addition to the source code generated by OpenAPI c generator, these two examples implement the config utility functions in their own source code.

int loadK8sConfigInCluster(char *token, int token_buf_size);
int init_k8s_connector(const char *token_out_of_cluster);

After this PR, I will implement and integrate the config utility to kuberentes-client/c. the same as other language client libraries.

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

ityuhui commented Mar 19, 2020

/cc @brendandburns

#include <stdio.h>
#include <errno.h>

#define K8S_APISERVER_BASEPATH "https://9.111.254.254:6443"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's anonymize this with something like "https://your.server.here"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to have been fixed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I have updated but not pushed to Github.

I want to

  1. Prepare a GNU Indent parameter list first and get your reviewed
  2. Use GNU Indent to check my new source
  3. Push the new source code to here for review again.

}

int
loadK8sConfigInCluster(char *token, int token_buf_size)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: function on the same line as the return type

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

return 0;
}

int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

char token_in_cluster[K8S_TOKEN_BUF_SIZE];
memset(token_in_cluster, 0, sizeof(token_in_cluster));

//loadK8sConfigInCluster(token_in_cluster, K8S_TOKEN_BUF_SIZE); // in cluster
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete commented out code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put the //loadK8sConfigInCluster here to tell user that how to get config when the program is in a kubernets cluster. It maybe confused.

Deleting it is OK. I can provide another example or function to demo getting config in cluster.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you add a comment like:

// If you want to load in cluster you would use
// loadk8sConfigInCluster(...)

That's fine. As it is currently written it's not quite clear.


char valueToken[K8S_TOKEN_BUF_SIZE];
memset(valueToken, 0, sizeof(valueToken));
//sprintf(valueToken, K8S_AUTH_VALUE_TEMPLATE, token); // in cluster
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

0 /* watch */
);
printf("return code=%ld\n", apiClient->response_code);
if(pod_list){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: spaces between if and (pod_list)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

printf("pod name=%s\n", pod->metadata->name);
}

}else{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too wrt spaces.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

}

int
loadK8sConfigInCluster(char *token, int token_buf_size)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function and return type on same line

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

if (fp == NULL) {
if (errno == ENOENT) {
printf("\
%s: The file %s does not exist.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is there a line-break here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No special reason, just want to keep the message text in a dedicated line.

Removing the line-break is OK to me.

char token_in_cluster[K8S_TOKEN_BUF_SIZE];
memset(token_in_cluster, 0, sizeof(token_in_cluster));

//loadK8sConfigInCluster(token_in_cluster, K8S_TOKEN_BUF_SIZE); // in cluster
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK


char valueToken[K8S_TOKEN_BUF_SIZE];
memset(valueToken, 0, sizeof(valueToken));
//sprintf(valueToken, K8S_AUTH_VALUE_TEMPLATE, token); // in cluster
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@brendandburns
Copy link
Contributor

Thanks for the examples!

I think we need to adopt a style guide for this repo, and add some validation testing to ensure that code matches the style guide.

Do you have any suggestions for a good C style guide? Perhaps Gnu Indent (https://www.gnu.org/software/indent/)

@ityuhui
Copy link
Member Author

ityuhui commented Mar 19, 2020

@brendandburns Thank you for your review. I will update the code by your comment.

And I agree that we should adopt a style guide for this repo. Let me learn and try Gnu Indent.

@ityuhui
Copy link
Member Author

ityuhui commented Mar 19, 2020

Hi @brendandburns

Do you have a preferred GNU Indent parameter list ?
(e.g. -npro -nip -nlp -npsl -i4 -ts4 -sob -l200 -ss -bl -bli 0)

If not, I will prepare a parameter list for your review.

@brendandburns
Copy link
Contributor

I don't have a preferred parameter list, let's start with what you suggest.

@brendandburns
Copy link
Contributor

Sounds great, let me know when this PR is ready to review again.

Thanks!

1. Anonymize the API server address

2. Remove useless line-break

3. Delete commented out code
@ityuhui
Copy link
Member Author

ityuhui commented Mar 20, 2020

Hi @brendandburns

I have prepared a parameter list of GNU Indent for your review:

Parameter List

-npro -nbad -nip -lp -npsl -npcs -i4 -ts4 -sob -br -ce -nut -bap -nbc -bbo -brs -cs -nfc1 -nfca -hnl -ip0 -nprs -saf -sai -saw -ci4 -cli0 -l200

Detail Description

Option Description
-npro Do not read ‘.indent.pro’ files.
-nbad Do not force blank lines after declarations.
-nip Zero width indentation for parameters.
-lp Line up continued lines at parentheses.
-npsl Put the type of a procedure on the same line as its name.
-npcs Do not put space after the function in function calls.
-i4 Set indentation level to 4 spaces.
-ts4 Set tab size to 4 spaces.
-sob Swallow optional blank lines.
-br Put braces on line with if, etc.
-ce Cuddle else and preceding ‘}’.
-nut Use spaces instead of tabs.
-bap Force blank lines after procedure bodies.
-nbc Do not force newlines after commas in declarations.
-bbo Prefer to break long lines before boolean operators.
-brs Put braces on struct declaration line.
-cs Put a space after a cast operator.
-nfc1 Do not format comments in the first column as normal.
-nfca Do not format any comments.
-hnl Prefer to break long lines at the position of newlines in the input.
-ip0 Indent parameter types in old-style function definitions by 0 spaces.
-nprs Do not put a space after every '(' and before every ')'.
-saf Put a space after each for.
-sai Put a space after each if.
-saw Put a space after each while.
-ci4 Continuation indent of 4 spaces.
-cli0 Case label indent of 0 spaces.
-l200 Set maximum line length for non-comment lines to 200.

Reference

GNU Indent Document

New Example Code

I have used GNU Indent with this parameter list to generate the new example source code and pushed them here for review again.

@brendandburns
Copy link
Contributor

Looks great!

/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 Mar 20, 2020
@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

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants