-
Notifications
You must be signed in to change notification settings - Fork 49
A few new endpoints and other improvements #65
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
Conversation
|
Hi @thomasjm. Thanks for the PR! I'm definitely interested in including the new API calls. I'm not sure about the rest of the changes though. The large diffs make code review difficult and I don't see a significant enough benefit. Maybe @denibertovic feels differently though. If you make a new PR with the API calls and relevant integration tests, I'll start reviewing your changes. |
|
Hi @jprider63 -- if you look at the individual commits they're pretty well organized, so you could cherry-pick whatever ones you like. I don't have time to break up the PR unfortunately. |
|
Another change I forgot to mention:
Happy to explain why any of the changes are good and awesome :) Another general question I had is if you've considered autogenerating the types in this library using the OpenAPI spec provided with Docker API v1.25 and above. I took a stab at this a while back but the codegen wasn't working well enough. However I noticed recently that haskell-kubernetes has gotten this to work. It would eliminate the manual work of writing down Haskell types for everything--theoretically you could support the full Docker API in one fell swoop. |
|
Sorry for the delayed reply everyone. I'm a bit swamped right now but I'd like to look at this over the weekend. Most of the changes are something I wanted to add at some point so I think it should be good. I'll comment again once I go through the changes. As far as generating the api goes....I thought about this at the start but the swagger api spec didn't exist then. There's a moby issue that's been open for forever. That being said, if that is done at some point I'd really like to switch to a generated approach with lenses and what not. |
|
At this point it's clear that we're not going to merge this. I should have been clear about this before instead of leaving the PR open to drag on like this....it's bad etiquette and that's totally on me. I just don't have the bandwidth and the PR was initially submitted with multiple distinct new features but intermixed with certain unrelated refactoring choices that would have been better left of for future PRs (like moving modules around) that make it difficult to review in one go with the limited time I have for this project. I really do appreciate your effort @thomasjm but I couldn't merge the PR in the state it was in back then and even more so now that it's drifted from master too much...so I'm going to close this PR. As for the network endpoints I think it's much more likely to be able to merge #70 cc @jprider63 |
Hi! I've made some improvements to this library in the last few days and wondered if you'd be interested in merging them. Changes include
-- New API calls:
listNetworks,inspectNetwork, andgetContainerStats-- Switch from cabal to hpack
-- Split
Types.hsinto separate files pertaining to networks, stats, etc.-- Split tests into separate files for unit tests and integration tests. Use hspec for integration tests because of better support for JUnit-style setup/teardown.