search

denibertovic / docker-hs

A Haskell library for the Docker Engine API
BSD 3-Clause "New" or "Revised" License
76 stars 48 forks source link

A few new endpoints and other improvements #65

Closed thomasjm closed 4 years ago

thomasjm commented 6 years ago

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, and getContainerStats -- Switch from cabal to hpack -- Split Types.hs into 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.

jprider63 commented 6 years ago

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.

thomasjm commented 6 years ago

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.

thomasjm commented 6 years ago

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.

denibertovic commented 6 years ago

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 was docker (later moby) issue that's been open for forever. What's worse I'm not sure how much we can trust that spec since most of it currently being used for docs only and the internal representation of the docker engine are not (all) implemented by using it. See here. What I think is happening is that if the types change they have a process for updating the spec manually. This might be good enough IDK. @jprider63 what do you think?

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.

denibertovic commented 4 years ago

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