[WIP] Added quantity format #540
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sreeram-venkitesh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
I think the introduction of "k8s.io/apimachinery/pkg/api/resource" import here would cause loop dependency issue later which we may wanna avoid. |
|
@cici37 Yes I was wondering the same. In order to use |
| package strfmt | ||
|
|
||
| import ( | ||
| "k8s.io/apimachinery/pkg/api/resource" |
There was a problem hiding this comment.
Unfortunately, this repo cannot depend on anything published out of kubernetes/kubernetes/staging, like k8s.io/apimachinery. We can't allow cycles between vendored dependencies used within kubernetes/kubernetes and things published out of kubernetes/kubernetes/staging.
(I haven't read the rest of the PR or the context, just followed the link to this from the apimachinery agenda meeting, but wanted to let you know this import isn't going to work before you got too far down this road)
|
|
||
| func init() { | ||
| quantity := Quantity("") | ||
| Default.Add("quantity", &quantity, isQuantity) |
There was a problem hiding this comment.
I wonder if this type definition / registration could be done from outside kube-openapi, e.g. defined in kubernetes/kubernetes or apiextensions-apiserver and then registered
If so, that would avoid the kube-openapi → k8s.io/apimachinery dependency
Part of kubernetes/kubernetes#130639 and along the same lines of #526.
I tested this locally but I'm not sure if we should add
k8s.io/apimachineryin vendor here.I have a corresponding PR in k/k which I'm opening along with these changes. Would love to hear some feedback.