-
Notifications
You must be signed in to change notification settings - Fork 142
docs: updates to source code doc #1032
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
ensure style guide compliance rewrites for clarity & readability add admonitions
|
@davidjohnbarton this is now in reviews |
drobnikj
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.
Just idea to keep node version always latest
| The following `Dockerfile`: | ||
|
|
||
| ```dockerfile | ||
| FROM apify/actor-node:16 |
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.
Let's update it to latest node js template with
apify/actor-node:20
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.
Or maybe better remove version and just keep with
apify/actor-node
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.
Based on proposed changes to dockerfile doc, I will put there apify/actor-node:20 for consistency across docs
| ``` | ||
|
|
||
| will build the Actor from the `apify/actor-node:16` image, copy the `package.json` and `package-lock.json` files to the image. | ||
| 1. Builds the Actor from the `apify/actor-node:16` base image. |
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.
| 1. Builds the Actor from the `apify/actor-node:16` base image. | |
| 1. Builds the Actor from the `apify/actor-node` base image. |
| 1. Builds the Actor from the `apify/actor-node:16` base image. | ||
| 2. Copies the `package.json` and `package-lock.json` files to the image. | ||
| 3. Installs the `npm` packages specified in `packages.json`, omitting development and optional dependencies. | ||
| 4. Copies the rest of the source code to the image | ||
| 5. Runs the `npm start` command defined in `package.json` |
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.
Question: Maybe it would make more sense to inline this as comments to the Dockerfile?
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've slightly restructured the whole doc around this part by:
First providing the whole code block and only after that breaking it up with explanations, @valekjo please let me know if this works better
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.
Nice! Thanks.
restructured the document adding new headings and ToC elements broke up the code examples for dockerfile and added explanations
|
@davidjohnbarton this is now post reviews and will be merged today , tomorrow at the latest |
ensure style guide compliance
rewrites for clarity & readability
add admonitions