-
Notifications
You must be signed in to change notification settings - Fork 230
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
Doc update for life cycle hook features #348
base: main
Are you sure you want to change the base?
Conversation
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.
Thank you for your contribution
I have added a comment for a peer review
| Property | Type|Description | | ||
| :--- | :--- | :-- | | ||
| `onCreateCommand` | [string, array, object](/docs/specs/devcontainerjson-reference.md#formatting-string-vs-array-properties)|First time inside the container immediately after it has started. | | ||
| `updateContentCommand` | [string, array, object](/docs/specs/devcontainerjson-reference.md#formatting-string-vs-array-properties)| First time inside the container after `onCreateCommand` whenever new content is available in the source tree during the creation process.| |
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.
@joshspicer do you think we should use the wordings consistent to the documentation here
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.
Would a link back to https://containers.dev/implementors/json_reference/#lifecycle-scripts on this page be sufficient clarity, @dfberry ?
I think reducing the amount of duplicated descriptions would be helpful to prevent information getting stale.
Regardless I agree on the consistency
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.
There is a link back, it below but not super obvious. I tried to be very respectful of the text in the other doc. I didn't know these other life cycle hooks where available because dev containers in vscode really only shows 1 by default. I was trying to make it more obvious but perhaps this is the wrong platform for that? Not sure. @seesharprun @bamurtaugh Thoughts?
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.
Thanks for opening this @dfberry. I love the goal @joshspicer mentions to avoid duplicated definitions and include links as possible, especially as we have content here and in containers.dev.
@dfberry maybe if the link wasn't apparent, another link could be added on top of the table, and/or links could be added to each entry in the table?
And speaking of containers.dev, for any change we pursue in this repo, could you also open a PR in the containers.dev repo when you get a chance: https://github.com/devcontainers/devcontainers.github.io/blob/gh-pages/_implementors/features.md? Thank you!
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.
@bamurtaugh Let me step back and walk through how I got here because I definitely think a link is missing from the devcontainer created by VSCode to this understanding of the different lifecycle events which @seesharprun alerted me to. I only knew about the 1 hook in the typical TS/JS/Node.js container. Once I got to this documentation for lifecycle properties, I had to figure out which lifecycle event was best suited to the typical usecases for scripts in the dev container.
Issues are:
- from devcontainers, how do I know and understand their are more lifecycle hooks than the 1 presented in the devcontainer
- once I get to the documentation, how do I understand use case for each event
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.
Thanks for clarifying. It sounds like an alternative option might be adding a link to these docs via a comment in the dev container Template or image, i.e. above this line https://github.com/devcontainers/templates/blob/main/src/cpp/.devcontainer/devcontainer.json#L16. Does that sound like it would've helped @dfberry?
No description provided.