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

Support to serialize MicroTime fields #830

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

uesyn
Copy link
Contributor

@uesyn uesyn commented Jun 25, 2022

Some Kubernetes objects expect to have date fields represented RFC3339 format with microsecond.
However, Kubernetes is unable to represent microsecond field in openapi schema.
ref: kubernetes/kubernetes#97904

In this PR, patch the kubernetes/gen which generate clients based on the open api schema generated by kubernetes, and define V1MicroTime to represent microseconds.
I confirmed the below code works.

import * as k8s from "@kubernetes/client-node";

async function main(): Promise<void> {
  const config = new k8s.KubeConfig();
  config.loadFromDefault();

  const cli = config.makeApiClient(k8s.CoordinationV1Api);
  await cli.createNamespacedLease("default", {
    metadata: {
      name: "foo",
      namespace: "default",
    },
    spec: {
      acquireTime: new k8s.V1MicroTime(),
      holderIdentity: "foo",
    },
  });
}

(async () => {
  await main();
})()

ref: #828

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 25, 2022
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 25, 2022
@uesyn uesyn changed the title Support to serialize micro date fields Support to serialize MicroTime fields Jun 25, 2022
@brendandburns
Copy link
Contributor

@uesyn thank you for the PR!

Instead of patching the pre-process python script, can you just send a PR to this repo:
https://github.com/kubernetes-client/gen/blob/master/openapi/preprocess_spec.py

I'm a maintainer on that repo so I can get that PR merged pretty quickly.

That will be cleaner/easier to understand.

Once that PR is in, we can rebase this one to pick up those changes and then merge this one.

Thanks!

@uesyn
Copy link
Contributor Author

uesyn commented Jun 28, 2022

@brendandburns Thank you for the comment!

I created the PR for kuberentes/gen!

I checked kubernetes/gen's master branch, and found that it's typescript's generatorName is typescript(for 1.x.y versions) not typescript-node(for 0.x.y versions).
ref: kubernetes-client/gen#219

Therefore, to generate codes with typescript-node, we need to create a new branch from this commit, and cherry-pick the PR to it.

@uesyn
Copy link
Contributor Author

uesyn commented Jun 28, 2022

Once that PR is in, we can rebase this one to pick up those changes and then merge this one.

I rebased this PR in advance that the PR is merged.
I dropped the commit to patch kubernetes/gen.
Should I drop the commit which regenerate the codes too?

@brendandburns
Copy link
Contributor

Thanks I LGTM'd the gen PR. You're right that we need to branch and pull it back to the commit that we use for generation, but for now let's just merge this PR also.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 29, 2022
@uesyn
Copy link
Contributor Author

uesyn commented Jun 29, 2022

@brendandburns
Thank you for the review of the kubernetes/gen PR!
I rebased this PR, and make it contain only V1MicroTime type definition. PTAL😄

@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 Jun 29, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2022
@k8s-ci-robot k8s-ci-robot merged commit d477db6 into kubernetes-client:master Jun 29, 2022
@brendandburns
Copy link
Contributor

brendandburns commented Jun 29, 2022

I think the next step is to run the Github action to regenerate the code.

I think that we should edit the github action to cherry-pick the gen commit before we run the generation.

@uesyn
Copy link
Contributor Author

uesyn commented Jun 30, 2022

@brendandburns
I have the same understanding.
If you don't mind, I will make PR 😄.

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

Successfully merging this pull request may close these issues.

None yet

3 participants