Skip to content
This repository was archived by the owner on Apr 24, 2024. It is now read-only.

Update Dev-Setup Documentation & Add docker folder#1241

Open
hatchla wants to merge 10 commits into
masterfrom
docker-setup
Open

Update Dev-Setup Documentation & Add docker folder#1241
hatchla wants to merge 10 commits into
masterfrom
docker-setup

Conversation

@hatchla

@hatchla hatchla commented Apr 3, 2024

Copy link
Copy Markdown
Contributor

Basics

  • The PR is rebased with current master
  • I added a line to changelog.md
  • Details of what I changed are in the commit messages
  • References to issues, e.g. close #X, are in the commit messages and changelog
  • The buildserver is happy

Checklist

  • I fully described what my PR does in the documentation
  • I fixed all affected documentation
  • I fixed the introduction tour
  • I wrote migrations in a way that they are compatible with already present data
  • I fixed all affected decisions
  • I added automated tests or a manual test protocol
  • I added code comments, logging, and assertions as appropriate
  • I translated all strings visible to the user
  • I mentioned every code or binary not directly written or done by me in reuse syntax
  • I created left-over issues for things that are still to be done
  • Code is conforming to our Architecture
  • Code is conforming to our Guidelines
  • Code is consistent to our Design Decisions
  • Exceptions to any guidelines are documented

Review

  • I've tested the code
  • I've read through the whole code
  • I've read through the whole documentation
  • I've checked conformity to guidelines
  • I've checked conformity to requirements
  • I've checked that the requirements are tested

@hatchla hatchla linked an issue Apr 3, 2024 that may be closed by this pull request
4 tasks
@hatchla hatchla changed the title Docker setup Update Dev-Setup Documentation & Add docker folder Apr 3, 2024
@hatchla hatchla added the please review Review by unspecified person requested label Apr 3, 2024
@hatchla

hatchla commented Apr 3, 2024

Copy link
Copy Markdown
Contributor Author

jenkins build please

@andreicristian97 andreicristian97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The files look ok. Could not test the docker setup for "docker+local" as locally I'm only running with the devcontainer, but instructions seem clear.

  • I've tested the code
  • I've read through the whole code
  • I've read through the whole documentation
  • I've checked conformity to guidelines
  • I've checked conformity to requirements
  • I've checked that the requirements are tested

@markus2330 markus2330 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great job. Version numbers somehow got lost. We aim to all use the same versions, to not have additional complexity by supporting multiple compiler versions.

Comment thread Makefile Outdated
Comment thread doc/development/01docker+local.md Outdated

## Installing Node 20 + Npm

Make sure node 20 is installed, you may use to easly install multiple versions of node [nvm](https://github.com/nvm-sh/nvm),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Try to not have several mentions of which versions to install, this is hard to maintain. (Add link to where the version is already mentioned).

Suggested change
Make sure node 20 is installed, you may use to easly install multiple versions of node [nvm](https://github.com/nvm-sh/nvm),
Make sure node, in the version as required for PermaplanT, is installed, you may use to easily install multiple versions of node [nvm](https://github.com/nvm-sh/nvm),

Comment thread doc/development_setup.md Outdated

## Ways to develop PermaplanT

Right now we have three different ways to develop Permaplant:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Right now we have three different ways to develop Permaplant:
Right now we have three different ways to develop PermaplanT:

Comment thread doc/development/01docker+local.md Outdated
This runs the Postgis database and PgAdmin.

```bash
cd docker

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
cd docker
cd .docker

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@horenso simply add/commit suggestion if it is okay.

Comment thread docker/.env

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

rename folder to .docker

@chr-schr chr-schr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Generally I really like the changes as well as separating the different approaches into one file each. 👍
Not sure about the docker container itself, most of it is duplicated with .devcontainer/

Comment thread doc/development/03local.md Outdated

## Install Rust

Follow the sets to [install Rust using rustup](https://www.rust-lang.org/tools/install).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Follow the sets to [install Rust using rustup](https://www.rust-lang.org/tools/install).
Follow the steps to [install Rust using rustup](https://www.rust-lang.org/tools/install).

Comment thread docker/docker-compose.yml

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it really necessary to more or less duplicate everything from .devcontainers/?
I usually just start the database part of the devcontainer with
docker compose -f .devcontainer/docker-compose.yml up db pgadmin -d
(but have add port forwarding of the db service 5432)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can also compose it from different compose files for .devcontainer and this setup, it's slightly different for instance it uses the host network

@@ -0,0 +1,30 @@
# Devcontainer

We are also supporting a containerized setup(docker/podman). For more information checkout the README inside [.devcontainer](https://github.com/ElektraInitiative/PermaplanT/blob/master/.devcontainer/README.md).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
We are also supporting a containerized setup(docker/podman). For more information checkout the README inside [.devcontainer](https://github.com/ElektraInitiative/PermaplanT/blob/master/.devcontainer/README.md).
We are also supporting a containerized setup(docker/podman). For more information checkout the README inside [.devcontainer](https://blob.permaplant.net/master/.devcontainer/README.md).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why can't we use relative links for files in the same repo with ..s?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Generally you can but it in many cases (e.g. links to outside of doc/) it won't work on doc.permaplant.net etc.

Comment thread doc/development/01docker+local.md Outdated
Comment on lines +11 to +12
Make sure node 20 is installed, you may use to easly install multiple versions of node [nvm](https://github.com/nvm-sh/nvm),
use apt or [download node directly](https://nodejs.org/en/download).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Make sure node 20 is installed, you may use to easly install multiple versions of node [nvm](https://github.com/nvm-sh/nvm),
use apt or [download node directly](https://nodejs.org/en/download).
Make sure node 20 is installed, you may use [nvm](https://github.com/nvm-sh/nvm) to easily install multiple versions of node, use apt or [download node directly](https://nodejs.org/en/download).

@horenso
horenso self-requested a review April 17, 2024 17:27

@horenso horenso left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It should be good now. We can deduplicate the docker setup in an extra PR but I think since this is dev setup only, it's not important.

@hatchla
hatchla requested review from chr-schr and markus2330 April 17, 2024 17:45
@hatchla

hatchla commented Apr 17, 2024

Copy link
Copy Markdown
Contributor Author

jenkins build please

@horenso

horenso commented Apr 18, 2024

Copy link
Copy Markdown
Contributor

The only thing touches is the docs and the .docker stuff, I don't know why the build fails...

@hatchla

hatchla commented Apr 19, 2024

Copy link
Copy Markdown
Contributor Author

@filo14 do you have an idea why the build always fails in this PR even though the failing files are not modified?

@markus2330

Copy link
Copy Markdown
Contributor

@hatchla Afaik @filo14 is already on holidays. Did you try to reproduce the issue locally? Is it maybe already present in master? In such cases, please report issues.

@hatchla

hatchla commented Apr 19, 2024

Copy link
Copy Markdown
Contributor Author

@hatchla Afaik @filo14 is already on holidays. Did you try to reproduce the issue locally? Is it maybe already present in master? In such cases, please report issues.

Was not reproduceable & not present in master.

@markus2330 markus2330 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Try to avoid duplication but still be more specific at which versions to use. It is probably best if these 3 variants are just a set of links to other parts of the documentation.

This runs the Postgis database and PgAdmin.

```bash
cd docker

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@horenso simply add/commit suggestion if it is okay.

@@ -0,0 +1,30 @@
# Devcontainer

We are also supporting a containerized setup(docker/podman). For more information checkout the README inside [.devcontainer](https://github.com/ElektraInitiative/PermaplanT/blob/master/.devcontainer/README.md).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Generally you can but it in many cases (e.g. links to outside of doc/) it won't work on doc.permaplant.net etc.


## Git user

Globally set git credentials are not available in the devcontainer, set them loaclly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Globally set git credentials are not available in the devcontainer, set them loaclly.
Globally set git credentials are not available in the devcontainer, set them locally.


## Background

(Devcontainer)[https://code.visualstudio.com/docs/devcontainers/containers] allows you to

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Markdown is the other way round. Please try to write one sentence per line and keep sentences shorter.

Install postgres, for instance on Ubuntu:

```bash
apt-get install libpq-dev postgresql-client

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this install the server?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This description is a bit useless? At least links to running frontend+backend should be provided?

@markus2330

Copy link
Copy Markdown
Contributor

Was not reproduceable & not present in master.

For me pre-commit run --all-files actually fails and hangs on master. See #832

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

please review Review by unspecified person requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve Development setup and docs

5 participants