Skip to content
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

Add license entries and dependency upper bounds #27

Closed
wants to merge 1 commit into from

Conversation

alexjg
Copy link

@alexjg alexjg commented Nov 1, 2018

I'm making a start on getting this into Hackage to help with #5 . I'm assuming one of the project maintainers will want to actually push the thing to Hackage. I'm getting a bunch of failing tests but they're also failing on master so will look into that separately.

All I've done here is add upper counds for dependencies as per the hackage guildelines and add a bunch of license references. Do we need to do anything else?

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Nov 1, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alexjg
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: brendandburns

If they are not already assigned, you can assign the PR to them by writing /assign @brendandburns in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 1, 2018
@guoshimin
Copy link
Contributor

The test failures are Just Null vs Nothing in roundtrip tests. I remember @jonschoning (author of the haskell module of swagger codegen) mention it at one point.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 1, 2018
@guoshimin
Copy link
Contributor

@alexjg Can you enlighten me on how you came up with these bounds? This is a question that has always puzzled me.

@jonschoning
Copy link
Contributor

fyi - since kubernetes/kubernetes.cabal is autogenerated, those changes would be lost by default on regen

@alexjg
Copy link
Author

alexjg commented Nov 1, 2018

@guoshimin sadly I basically guessed them by just pulling whatever the current latest on Hackage is. If there's another way of doing it I would be happy to do that. As far as I can tell though dependency resolution problems like this are hard, boring, and not going away 🙁

@jonschoning Ah, for some reason didn't realise the kubernetes.cabal was autogenerated. Shall I move this stuff to package.yaml as with the other subprojects?

General FYI: I'm not an expert at Haskell packaging or anything, just keen to use this project and happy to do the donkey work to get it pushed to Hackage so just let me know things that need doing.

@alexjg
Copy link
Author

alexjg commented Nov 2, 2018

@jonschoning How exactly is kubernetes/kubernetes.cabal generated? There's no script in the repo that I can see so I assume by manually runing swagger-codegen?

@guoshimin
Copy link
Contributor

guoshimin commented Nov 2, 2018 via email

@jonschoning
Copy link
Contributor

Is the intention to publish 5 packages to Hackage? How will users know they are related, or which ones to use?

@alexjg
Copy link
Author

alexjg commented Nov 2, 2018

Good point, I had assumed multiple packages based on the existing directory layout. If we want to merge them I guess we would need to do some restructuring. Alternatively, multiple packages doesn't seem to be uncommon, e.g Servant has servant, servant-server, servant-client etc.

@jonschoning
Copy link
Contributor

I don't think they can be merged into 1, at least not the auto-gen'd package. but users will have to know to use kubernetes-client-helper as the entry point somehow

@jonschoning
Copy link
Contributor

jonschoning commented Nov 2, 2018

Also, not sure if the root kubernetes name should be reserved on hackage, or if it should be qualified/prefixed/suffixed somehow

@guoshimin
Copy link
Contributor

I was thinking we can have two packages: kubernetes-openapi that is auto generated, and all the hand-written code in another package. @jonschoning you made some change to swagger-codegen to add the ability to customize the package name, right? And we can write a README.md page for the codegen'ed package to point users to the entrypoint and configure the codegen process to not override the README.md.

@alexjg sorry this is turning into kind of a rabbit hole :p

@jonschoning
Copy link
Contributor

jonschoning commented Nov 2, 2018

yes, the generator can customize the base module name and the cabal package name, and cabal package version independantly with CLI params

 -DbaseModule=XXX -DcabalPackage=YYY

you can view all the config params with the arguments

config-help -l haskell-http-client

@alexjg
Copy link
Author

alexjg commented Nov 3, 2018

@guoshimin Fine by me, I'm happier to go down a rabbit hole then for no one to respond :)

How do you propose we configure the codegen process to not overwrite README.md?

@guoshimin
Copy link
Contributor

Looks like it's already configured to not overwrite README.md here https://github.com/kubernetes-client/haskell/blob/master/kubernetes/.swagger-codegen-ignore

@alexjg
Copy link
Author

alexjg commented Nov 5, 2018

Okay that makes sense. Would that mean consolidating kubernetes-watch, kubernetes-client-helper, and kubeconfig into the proposed kubernetes package?

@alexjg alexjg mentioned this pull request Nov 12, 2018
@denibertovic
Copy link

What is currently missing for getting this published? I'd be happy to help if you can direct me where help is needed.

Regarding the naming I think the amazonka and gogol libraries are good examples of how to approach the split and naming. I propose we call the main package kubernetes which would include the bare minimum for a working setup and would at least include kubernetes-core and kubernetes-client.

@guoshimin
Copy link
Contributor

Sorry it's my fault for not moving it forward. Haven't been able to find time to work on it recently. The plan it to have a package named kubernetes-openapi that's auto-generated, and another yet-to-be-named package that contain all the hand-written code. I'll try to get the first part done in the near future.

@denibertovic
Copy link

@guoshimin no worries...just wondering if I can help somehow. Let me know.

@denibertovic
Copy link

@guoshimin is this PR still relevant given that #31 was merged?

@guoshimin
Copy link
Contributor

yeah, the changes here need to be adapted for #33

@guoshimin
Copy link
Contributor

@alexjg Do you want to take another look at this? The package structure is now stable.

@alexjg
Copy link
Author

alexjg commented May 15, 2019

Yep, happy to have another look. As far as I can see there are only two libraries missing version bounds. Now (unordered-containers and case-insensitive). I'm not sure how much of an issue that is for packaging but if it is an issue it will require changes in https://github.com/OpenAPITools/openapi-generator/ right?

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 13, 2019
@guoshimin
Copy link
Contributor

Yes, ideally.

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 14, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 12, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 12, 2019
@k8s-ci-robot
Copy link
Contributor

@alexjg: PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2019
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants