-
Notifications
You must be signed in to change notification settings - Fork 8.1k
compose sdk: release follow-up #23803
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
✅ Deploy Preview for docsdocker ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
glours
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👌
Thanks @aevesdocker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request: some more usage examples. Maybe take inspiration from the engine sdk examples page: https://deploy-preview-23751--docsdocker.netlify.app/reference/api/engine/sdk/examples/
Showing a few common use cases is probably a good idea to help people get going.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIT the SDK functions are self-explanatory as there's a 1:1 match with compose commands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could say that about the engine SDK too but I still think examples are useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong, in many places the docker CLI relies on multiple API calls to achieve features people know well.
I'm not against examples, but I don't want to just write useless stuff like
// example: List Compose applications
applications, err := compose.List(ctx, ListOptions{})
I'd be happy to add more example once we get feedback on API usability from actual users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah so when I say example, I don't mean showing function signatures. But we should be able to show some use cases beyond hello world. For example, using the EventProcessor. Or something with watch events, or custom logging, or even something more advanced like dependency visualization. Something to a) demo interesting capabilities, and b) show how these are meant to be used!
| ## Tracking operations with `EventProcessor` | ||
|
|
||
| The `EventProcessor` interface allows you to monitor Compose operations in real-time by receiving events about changes | ||
| applied to Docker resources such as images, containers, volumes, and networks. This is particularly useful for building | ||
| user interfaces, logging systems, or monitoring tools that need to track the progress of Compose operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repeating myself here but an example here would greatly help. It's quite opaque at the moment, hard to understand how this is meant to be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger, and I agree. Can I request that we get this PR merged now and I add and improve the docs in a separate PR? It's going to take me a bit more time to get that together and it'd be good to get what we have now live. I've addressed your other comments already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend git mv to ensure history is kept for this file. The history for the current file goes back to 2017.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I actually just moved this content onto this new page and kept the old page (I didn't rename the file or anything), so is git mv still needed? isn't the history for release-notes.md still kept in this instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @dvdksn thanks for approving, just want to check the thinking on this before I merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hopefully we can remove the release notes from the website and just have them on github anyway, in which case this doesn't matter 😄
Description
Follow-up to #23802
Related issues or tickets
Reviews