-
Notifications
You must be signed in to change notification settings - Fork 59
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
new package name and module base for generated code #31
new package name and module base for generated code #31
Conversation
@denibertovic can you review this PR? Thanks! |
Is there anything that can be done to help with this? I'm interested in using this library, and it'd be nice to have some of the moving parts get settled before it gets pushed to Hackage. |
You can review this PR :)
…On Thu, Feb 21, 2019, 12:42 PM Joe Kachmar ***@***.***> wrote:
Is there anything that can be done to help with this? I'm interested in
using this library, and it'd be nice to have some of the moving parts get
settled before it gets pushed to Hackage.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#31 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALPcHY8nPq8JZ08FBLEOUMaIf3H8u-s5ks5vPwSggaJpZM4abEUB>
.
|
Okay, so I did a brief review of the changes (since this nearly all generated code), as well as having run this locally. This seems like a good update, however there are two things that concern me in general:
Parts of (1) discussed in #28; unfortunately I can't seem to get the I'm unsure what the best way to approach (2) is; is the Kubernetes API backwards compatible between minor versions (e.g. |
@jkachmar Thanks for taking a look! For openapi-generator, I think we need to wait for kubernetes-client/gen#93 to be resolved. I took a look a while ago, and it didn't seem easy to switch to openapi-genenrator just for haskell. For 2) I think we should absolutely generate code for a more recent API version. Didn't do it here because I didn't want the two changes to be mixed together. |
It looks like kubernetes-client/gen#95 was already merged to switch python-asyncio, but we need to understand the cause of the previously linked build errors first. |
@jonschoning I think that might have been my error; I was using an old commit hash for I'm trying to rerun all this now, but I'm unfortunately running into this when trying to build the Log Snippet
|
Perhaps they need to be renamed as well e.g. not sure if the files & folder structure at the root should also be updated, but would certainly at least be more consistient. let me know if you'd like me to help out cleaning stuff up & add commits |
Agh, you're right I didn't even think to look in either of those modules, they'd break with the new naming scheme for sure 😞 Regarding packaging, this comment seems to lay out a fairly good structure. Assuming I'm interpreting it correctly:
This way if someone wants to use just the bindings, they're capable of doing so, but for most people the happy path of a batteries included Kubernetes client is easily accessible. |
That is a bit more structured and uses different package names than what guoshimin proposed, and isn't what is in this PR currently, but I do like the ergonomics of it better. Re-exporting the codegen under a single package definitely would make discoverability much easier, than having know there are several related-but-differently-named packages that need to be used together. |
@jkachmar Your proposal is better. I'll change this PR to do that! |
…en. adapt existing code to the change
PTAL |
Looks good! I suppose it makes the most sense to merge this PR, then restructure the module hierarchy? After that, if we can reconcile with the new |
What do you mean by restructure the module hierarchy? I will combine the other packages into a single one, is there something else? |
What I meant by this comment was that it might be more ergonomic to have a single package for the generated code (i.e. Rather than containing/publishing four separate packages, this repository could contain two:
Does this make sense? |
Yeah, sounds good! |
/lgtm |
/approve |
1 similar comment
/approve |
[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:
Approvers can indicate their approval by writing |
New package name: kubernetes-openapi
New module base: Kubernetes.OpenAPI
Generated using kubernetes-client/gen@8237ccb
On my system I had to run the command under sudo: