README update of Tomcat environment setup. #1607
Conversation
|
|
There was a problem hiding this comment.
Thanks for your contribution. While I appreciate your efforts in trying to improve this documentation, I don't see a great benefit from the changes as they are currently presented.
I stopped my review half-way through, please go ahead and either:
- vastly refactor the PR so that only minimal changes are applied (e.g.: only fixing actual errors / adding critical missing info to the documentation)
- close the PR
dev/README.md
Outdated
| apply selected AMPs | ||
|
|
||
| ## Setting up your development environment | ||
| ## 1. Setting up your development environment |
There was a problem hiding this comment.
I don't think this should be a numbered list, a lot of these "steps" are not really steps and are optional depending on the use-case or people may prefer to work in a different way. I think the previous approach with them just being regular paragraphs was more true to the nature of this documentation.
dev/README.md
Outdated
| @@ -1,70 +1,61 @@ | |||
| # Development Tomcat Environment | |||
|
|
|||
| It is possible to use Docker containers to test your code, but it is normally more convenient to simply run the repository webapp (**`alfresco.war`**) and Share webapp (**`share.war`**) in a tomcat instance. Options are also available to apply selected AMPs. | |||
There was a problem hiding this comment.
A few changes such as this one include very minimal rewording (e.g.: in this case just adding a final . to the sentence) at the cost of losing the formatting that is more convenient to easily scan and maintain the documentation.
Please let's retain the original formatting / try to not go overboard with the length of a single line. Apply a vertical guide line via your IDE to do so if needed, to not break the previous one.
This comment applies to ALL changes in this PR.
dev/README.md
Outdated
|
|
||
| Aliases ending in `D` provide Maven commands for building local Docker images. The AMPS environment variable will be of | ||
| interest, if you wish to build AMPs included in the repo and share projects. | ||
| Aliases ending in **`D`** provide Maven commands for building local Docker images. The AMPS environment variable will be of interest, if you wish to build AMPs included in the repo and share projects. |
There was a problem hiding this comment.
No need to make code snippets bold. They have great visibility already, depending on your client's rendering some people might actually find it easier to read non-bold code snippets. (applies to several changes in this PR).
dev/README.md
Outdated
|
|
||
| The `aliases` file includes a more detailed description. | ||
| ~~~ | ||
| **You may also build **`alfresco-community-repository`**, **`alfresco-community-share`** and **`alfresco-community-packaging`** with Docker images at once with **`comD`**.** |
There was a problem hiding this comment.
I think there's no point in explaining this specific alias here, there's many aliases and they should be described appropriately in the aliases file already so that's the source of truth for this kind of information. We don't want to duplicate this so we'd have to maintain it in 2 separate places in case some of the aliases change.
dev/README.md
Outdated
| done so already), so that your changes are in the community alfresco.war file. | ||
| ~~~ | ||
| ## 3. Build the Repo project | ||
| Build the `alfresco-community-repo` projects with **`comR`**, so that your changes are in the community alfresco.war file. |
There was a problem hiding this comment.
I think this rewording here is unnecessary, comR is already mentioned as an alternative in the presented code block that explains how to build the community repo.
dev/README.md
Outdated
| ~~~ | ||
| # The `entS` alias is the same as the following commands: | ||
| ## 4. Build the Share project | ||
| Build the `alfresco-community-share` project with **`comS`**, so that your changes are in the community share.war file, which also depends on your **`alfresco-community-repo`** project version. |
There was a problem hiding this comment.
Same as above, the added note about # The `comS` alias is the same as the following commands: should be sufficient.
dev/README.md
Outdated
| The simplest way to create these, is to use the `docker-compose.yml` file in the `dev` directory. | ||
| ~~~ | ||
| ## 5. Docker test environment | ||
| The repository code will need to talk to other ACS components, such as a database, message queue and transformers. The simplest way to create these, is to use the **`docker-compose.yml`** file in the **`dev`** directory. Do it with **`envUp`** alias. |
There was a problem hiding this comment.
Unnecessary rewording IMO, the envUp alias is already mentioned just bellow.
|
Hello, thank you for your feedback. I see the errors I've made and things that were not necessarily needed. I did my best to fix it and even though now it's not that different from original, I've decided to stay with a few changes:
Please let me know if everything's OK now. |
Changed:
xyzapostrophes by bolding,Added: