feat(ws): Notebooks 2.0 // Backend // add count to workspacekind sche…

[roee1313]

This PR solves issue #352

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kimwnasptd for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jiridanek]

/ok-to-test

[andyatmiami]

I managed to (briefly) talk with Mathew during the WG call today (Thursday, May 29th) and he stated he was comfortable embedding this "counts" information more naturally inline with the response schema (and not worrying about adhering to a 1:1 structural mapping w.r.t how the information is presented from k8s)

The below image represents my proposal on how this "embedding" could work. A couple notes:

• The same schema should be applied to the "list" as well as the "get-by-id" endpoints
  • For sake of brevity - I am only showing the "get-by-id" payload below
• I opted to "co-locate" the counts information closest to relevant data within the response. This is certainly most clean for the values arrays - but then leads to a root workspacesCount attribute at the root of the WorkspaceKind element as there isn't a more suitable place for it.
• For sake of consistency/readability - I chose to re-use the same attribute name throughout (workspacesCount) - with the "pattern" being <child resource name (plural)>Count... in the event this paradigm crops up elsewhere in the API - we could reuse this design philosophy
• Certainly open to other's opinions on if this is the most appropriate design. I had considered a counts: {....} object as well... but chose not to advocate for that option as its too redundant/nested (having to repeat context like imageConfig as well as the IDs in order for a client to then have to perform the mapping to relevant object to display inline as we do in our table view today)

FYI: @thesuperzapper @ederign @paulovmr

[google-oss-prow]

@andyatmiami: changing LGTM is restricted to collaborators

In response to this:

I managed to (briefly) talk with Mathew during the WG call today (Thursday, May 29th) and he stated he was comfortable embedding this "counts" information more naturally inline with the response schema (and not worrying about adhering to a 1:1 structural mapping w.r.t how the information is presented from k8s)

The below image represents my proposal on how this "embedding" could work. A couple notes:

• The same schema should be applied to the "list" as well as the "get-by-id" endpoints
  • For sake of brevity - I am only showing the "get-by-id" payload below
• I opted to "co-locate" the counts information closest to relevant data within the response. This is certainly most clean for the values arrays - but then leads to a root workspaces_count attribute at the root of the WorkspaceKind element as there isn't a more suitable place for it.
• For sake of consistency/readability - I chose to re-use the same attribute name throughout (workspaces_count) - with the "pattern" being <child resource name>_counts... in the event this paradigm crops up elsewhere in the API - we could reuse this design philosophy
• Certainly open to other's opinions on if this is the most appropriate design. I had considered a counts: {....} object as well... but chose not to advocate for that option as its too redundant/nested (having to repeat context like imageConfig as well as the IDs in order for a client to then have to perform the mapping to relevant object to display inline as we do in our table view today)

FYI: @thesuperzapper @ederign @paulovmr

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[roee1313]

Hey @andyatmiami
I added the required fixes, and they are ready for to be checked,
Thanks!