Merging hand-written code into one package #33

guoshimin · 2019-02-28T00:24:12Z

No description provided.

guoshimin · 2019-02-28T00:31:01Z

jkachmar · 2019-02-28T20:01:33Z

Two comments:

How do you feel about taking the restructuring as an opportunity to make the module naming a bit more consistent? For example:
- Kubernetes.OpenAPI.ClientHelper -> Kubernetes.Client.Helper
- Kubernetes.OpenAPI.Watch.Client -> Kubernetes.Client.Watch
- Kubernetes.OpenAPI.KubeConfig -> Kubernetes.Client.KubeConfig

With this naming scheme, all of the hand-written functions are under the Kubernetes.Client namespace, and all of the generated functions are under the Kubernetes.OpenAPI namespace.

I haven't given this too much thought though, so there might be good reasons not to do it this way.

Is there a better name for the ClientHelper module? I frequently see Helper or Util modules on Hackage and they can feel fairly vague. Perhaps it would be better to have something like:
-Kubernetes.OpenAPI.KubeConfig -> Kubernetes.Client.KubeConfig
- Kubernetes.OpenAPI.ClientHelper -> Kubernetes.Client.Config
  ...where the KubeConfig module holds functions/types concerned with Kubernetes kubeconfig functionality and the Config module holds functions/types concerned with the client configuration (i.e. the certificate and manager stuff that's currently in there).

guoshimin · 2019-03-01T21:41:36Z

Yeah I'll make these changes.

guoshimin · 2019-03-01T22:02:17Z

PTAL

jkachmar

Looks awesome, thanks very much!

guoshimin · 2019-03-01T22:14:44Z

@brendandburns can you approve? Thanks!

denibertovic · 2019-03-08T22:41:39Z

kubernetes-openapi-client/README.md

+
+import           Data.Function           ((&))
+import qualified Kubernetes.OpenAPI.API.CoreV1
+import           Kubernetes.OpenAPI.Client       (dispatchMime)


@guoshimin I don't think the README example here reflects the lates module name changes.

denibertovic · 2019-03-08T22:45:42Z

@guoshimin @jkachmar The way I usually approach module naming in this situation is to have "Kubernetes.Client." but then re-export whatever I need to "Kubernetes.Client" that way a user of the library only needs to do "import Kubernetes.Client" and they're set to go. Hoever, a more experienced user is free to pick and choose and import stuff from "Kubernetes.Client." one by one (for whatever reason).

I don't have an opinion on the names in "Kubernetes.Client.*" as long as the re-export in "Kubernetes.Client" give me as a user all I need.

Does this make sense?

denibertovic · 2019-03-08T23:05:16Z

One more question/suggestion regarding the package names: Isn't it a little too much to name the 2 packages kubernetes-openapi-client-gen and kubernetes-openapi-client? Seems a bit of a mouthful. Wouldn't it make more sense to give them shorter (memorable names) like kubernetes-core and kubernetes-client (or event better just kubernetes? I understand the allure to suffix -gen so folks know it's generated code but 2 prominent examples of such code, amazonka and gogol, don't seem to use this scheme and it makes them much more memorable and more intuitive to use.

EDIT: Just to be clear this doesn't impact this PR it's just a suggestion. If however you would like to eventually make this change I have some free time this weekend and would be able to work on it.

guoshimin · 2019-03-09T00:47:39Z

@denibertovic Thanks for the feedback! PTAL.

denibertovic · 2019-03-09T23:03:54Z

@guoshimin LGTM now.

Do you have any feedback on the package names in my second comment (for another PR of course).

guoshimin · 2019-03-10T07:40:13Z

Oops, missed your second comment. @jonschoning first raised the point whether it was wise to take the name kubernetes on hackage and I agreed with him, so we made the package name more qualified. It's possible that we have gone too far. Now that I think about it more, it may be better to have just one package that includes both generated and hand-written code. That's how the other languages under kubernetes-client work, and more intuitive for the user. One complication is that we will have one .cabal file for both generated and hand-written code, and we will need to maintain it manually, but I think it's outweighed by the benefits. So @jonschoning @jkachmar what do you guys think of just having one package named kubernetes-client?

denibertovic · 2019-03-10T17:52:56Z

@guoshimin why would it not be wise to use the name "kubernetes" if it's the official api bindings? Please point me to a discussion about this so you don't have to repeat yourself.

That said, I'm 👎 on the single package proposal. Having 2 packages seems perfectly reasonable and quite in spirit for these types of things in the ecosystem.

FWIW, if the "qualified" name must stay I would vote for keeping the kubernetes-openapi-client-gen name but renaming kubernetes-openapi-client to just kubernetes-client.

jkachmar · 2019-03-10T18:45:15Z

👍 for shorter names (e.g. kubernetes-client and kubernetes-client-core/kubernetes-client-gen)
👎 for a single package
- I feel like if the generated stuff is separate, it's a little easier for someone to run the generator on their own (e.g. for a different version of the API) in a forked version

jonschoning · 2019-03-10T19:12:25Z

I'm good with kubernetes-client
- Just to add some context, my reason for avoiding a root name like kubernetes is that although the code in this repo may be generated from the official swagger openapi spec, the openapi generator itself doesn't know anything about kubernetes at all and is constrained to generate to the lowest common denominator of "forall" OAS specs.
  Packages like gogol and amazonka have tailor-made generators, which have the advantage of being able to customize the codegen specifically for their own use case.
  So I was thinking to just allow some room on hackage, if someone in the future decides to implement tailor-made generator for kubernetes and really improve the UX.
  But I can live with kubernetes-client which I think is different enough
I think I prefer keeping the generated code in a separate package as well, but am not super opinionated about this one.

guoshimin · 2019-03-11T16:53:15Z

Thank you folks for your input. I'll change the package name to kubernetes-client/kubernetes-client-core.

guoshimin · 2019-03-11T17:08:15Z

PTAL

denibertovic · 2019-03-11T18:00:56Z

kubernetes-client/package.yaml

+name: kubernetes-client
+version: 0.1.0.0
+description: |
+  This package contains functions for working with kubeconfig files.


This description should get updated.

Good catch. Done. And added a few more fields.

denibertovic · 2019-03-11T18:02:16Z

kubernetes-client/package.yaml

+  - x509-system
+  - x509-store
+  - x509-validation
+executables:


I don't think when installing kubernetes-client that I should have to build this example binary.

Moved to test.

denibertovic · 2019-03-11T18:02:47Z

kubernetes/README.md

@@ -8,4 +8,4 @@ OpenAPI-Specification: https://github.com/OAI/OpenAPI-Specification/blob/master/

 ## Usage

-Please refer to the README of the `kubernetes-client-helper` package.
+Please refer to the README of the `kubernetes-openapi-client` package.


s/kubernetes-openapi-client/kubernetes-client/

denibertovic · 2019-03-11T18:04:06Z

kubernetes-client/package.yaml

+  - data-default-class
+  - http-client >=0.5 && <0.6
+  - http-client-tls
+  - kubernetes-openapi-client-gen == 0.1.0.0


This should change to kubernetes-client-core right?

I will update it when I regen the code to use the new package name.

denibertovic · 2019-03-12T00:24:31Z

LGTM. 👍

brendandburns · 2019-03-12T01:02:30Z

/lgtm
/approve

k8s-ci-robot · 2019-03-12T01:02:34Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

guoshimin2 added 3 commits February 27, 2019 16:09

merging hand-written code into one package

299d137

fix module names

cfca009

fix stack.yaml

a6d456d

k8s-ci-robot requested review from brendandburns and mbohlool February 28, 2019 00:24

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 Feb 28, 2019

fix

3ccfa93

rename modules

faa59d5

jkachmar approved these changes Mar 1, 2019

View reviewed changes

denibertovic reviewed Mar 8, 2019

View reviewed changes

fix README. re-export modules. add example

0f4c72b

guoshimin mentioned this pull request Mar 9, 2019

Add license entries and dependency upper bounds #27

Closed

rename

19fc1c3

denibertovic reviewed Mar 11, 2019

View reviewed changes

guoshimin2 added 2 commits March 11, 2019 11:34

fixes to package.yaml

e9c530b

only do typechecking

d367033

k8s-ci-robot assigned brendandburns Mar 12, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 12, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2019

k8s-ci-robot merged commit de91fe9 into kubernetes-client:master Mar 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merging hand-written code into one package #33

Merging hand-written code into one package #33

guoshimin commented Feb 28, 2019

guoshimin commented Feb 28, 2019

jkachmar commented Feb 28, 2019

guoshimin commented Mar 1, 2019

guoshimin commented Mar 1, 2019

jkachmar left a comment

guoshimin commented Mar 1, 2019

denibertovic Mar 8, 2019

denibertovic commented Mar 8, 2019 •

edited

Loading

denibertovic commented Mar 8, 2019 •

edited

Loading

guoshimin commented Mar 9, 2019

denibertovic commented Mar 9, 2019

guoshimin commented Mar 10, 2019

denibertovic commented Mar 10, 2019 •

edited

Loading

jkachmar commented Mar 10, 2019

jonschoning commented Mar 10, 2019

guoshimin commented Mar 11, 2019

guoshimin commented Mar 11, 2019

denibertovic Mar 11, 2019

guoshimin Mar 11, 2019

denibertovic Mar 11, 2019

guoshimin Mar 11, 2019

denibertovic Mar 11, 2019

guoshimin Mar 11, 2019

denibertovic Mar 11, 2019

guoshimin Mar 11, 2019

denibertovic Mar 12, 2019

denibertovic commented Mar 12, 2019

brendandburns commented Mar 12, 2019

k8s-ci-robot commented Mar 12, 2019

Merging hand-written code into one package #33

Merging hand-written code into one package #33

Conversation

guoshimin commented Feb 28, 2019

guoshimin commented Feb 28, 2019

jkachmar commented Feb 28, 2019

guoshimin commented Mar 1, 2019

guoshimin commented Mar 1, 2019

jkachmar left a comment

Choose a reason for hiding this comment

guoshimin commented Mar 1, 2019

Choose a reason for hiding this comment

denibertovic commented Mar 8, 2019 • edited Loading

denibertovic commented Mar 8, 2019 • edited Loading

guoshimin commented Mar 9, 2019

denibertovic commented Mar 9, 2019

guoshimin commented Mar 10, 2019

denibertovic commented Mar 10, 2019 • edited Loading

jkachmar commented Mar 10, 2019

jonschoning commented Mar 10, 2019

guoshimin commented Mar 11, 2019

guoshimin commented Mar 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denibertovic commented Mar 12, 2019

brendandburns commented Mar 12, 2019

k8s-ci-robot commented Mar 12, 2019

denibertovic commented Mar 8, 2019 •

edited

Loading

denibertovic commented Mar 8, 2019 •

edited

Loading

denibertovic commented Mar 10, 2019 •

edited

Loading