Skip to content

Conversation

@tg123
Copy link
Member

@tg123 tg123 commented Feb 20, 2023

The Problem

The k8s models now have explicit converter for converting model objects in different versions, e.g. V1 <-> V1beta1
Typical usage is V1X v1obj = (V1X)v1beta1obj

The converter is based on AutoMapper and introduces the dependency to AutoMapper which stopped supporting netstandard2.0. Removing the dependency would also make the sdk clean and more secure.

In additional, seems the feature is barely used.

New Design

  • a new KubernetesClient.ModelConverter lib for those who want to use version converters and move AutoMapper to this lib.
  • users should call ModelVersionConverter.Converter = AutoMapperModelVersionConverter.Instance; to set Converter or it will throw an error
  • ModelVersionConverter.Converter is an interface IModelVersionConverter. users could impl it and set ModelVersionConverter.Converter to their own converter

@k8s-ci-robot k8s-ci-robot added 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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 20, 2023
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 20, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 20, 2023
@brendandburns
Copy link
Contributor

@tg123 this seems fine to me. Do you think this is ready to merge?

@tg123
Copy link
Member Author

tg123 commented Feb 28, 2023

I am good. Please also add a nuget lib KubernetesClient.ModelConverter before merging.

@brendandburns
Copy link
Contributor

/lgtm
/approve

I think it will "just work" because the KubernetesClient package already exists. We'll see. If not I will upload the artifact.

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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,tg123]

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 abf6950 into kubernetes-client:master Mar 3, 2023
@brendandburns
Copy link
Contributor

sigh looks like it didn't, I will build/update this nuget manually.

@brendandburns
Copy link
Contributor

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants