Skip to content
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

Update readme.md #4001

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Update readme.md #4001

wants to merge 3 commits into from

Conversation

bakeiro
Copy link
Contributor

@bakeiro bakeiro commented Mar 12, 2024

documentation change, in order to improve clarity (for the dev experience)

removed frontend documentation for more clarity
@@ -31,8 +28,6 @@ Open a command line or terminal, and navigate to the `app` folder (where this `r

Install `docker` from <https://docs.docker.com/engine/install/>.

Install `docker-compose` from <https://docs.docker.com/compose/install/>.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merged in docker since July 2023

@@ -51,7 +46,7 @@ An alternative on **macOS** is to install the tools locally with `brew install g
If you are on **Windows** (without WSL2), run the following command:

```sh
docker run --pull always --rm -w /app -v %cd%:/app registry.gitlab.com/couchers/grpc sh -c "cat generate_protos.sh | dos2unix | sh"
docker run --pull always --rm -w /app -v ${PWD}/app:/app registry.gitlab.com/couchers/grpc sh -c "apt-get update && apt-get install dos2unix && cat generate_protos.sh | dos2unix | sh"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solved command on windows

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If dos2unix is not in the container, then I think we should add it there instead of installing it here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, thing is, I don't have access to it 🙃

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've installed it there now, so no need to install here!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okey, thank you!!

@@ -11,9 +11,6 @@ To **run the app locally**, you need to do **four things**:
1. Get the code, navigate to the `app` folder and install `docker` and `docker-compose`
2. Compile the protocol buffers
3. Launch the backend with `docker-compose`
4. Install and launch the web frontend with `yarn`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep this step, but instead include a link to the front-end repo, e.g.:
Install and launch the web frontend

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right!

@@ -11,9 +11,6 @@ To **run the app locally**, you need to do **four things**:
1. Get the code, navigate to the `app` folder and install `docker` and `docker-compose`
2. Compile the protocol buffers
3. Launch the backend with `docker-compose`
4. Install and launch the web frontend with `yarn`

Are you only developing on the web frontend? If you don't want to install docker, you can follow the alterative instructions in `web/readme.md`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep this, but update it to say something like:
Are you only developing on the web frontend? You can run the frontend by itself, just follow instructions outlined in "add link here"

Copy link

@nciemniak nciemniak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good apart from the points I highlighted above 👍

@@ -11,9 +11,6 @@ To **run the app locally**, you need to do **four things**:
1. Get the code, navigate to the `app` folder and install `docker` and `docker-compose`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update here as well to instruct them to only install docker

@aapeliv aapeliv added the backend This issue relates to the python backend label Apr 20, 2024
@schradert schradert added the docs label May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend This issue relates to the python backend docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants