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
review of the setup document & fix typos in ld article #53
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.
I approve for the json-ld.md, but have some comments (see the line-based) on setup/index.md.
Here we override the default configuration which resides at `/pygeoapi/local.config.yml` within the Container | ||
with our local file [default.config.yml](https://github.com/geopython/pygeoapi/blob/master/docker/default.config.yml) | ||
by using Docker Volume Mount (`-v` option). A Docker Volume mount attaches, 'mounts', a | ||
In the upcoming exercises we are going to update the configuration file multiple times. |
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.
Hmm, in reality there is already a pygeoapi config in the workshop materials (now under docker/pygeoapi/docker.config.yml) which is mounted via docker-compose and modified in the exercises.
Suggestion is to mount that file in this exercise here.
Also think we should have a Docker Compose with that file mounted in this section as to prepare for "first.md"
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
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.
it could also make sense to first run directly on a container, before going into the compose use case
fix typos in ld article
c4d5a33
to
4b78e61
Compare
!!! question "Docker in the background" | ||
|
||
1. Type `docker-compose up -d` | ||
1. Type `docker ls`; verify that the pygeoapi container is running |
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.
Wrong earlier: must be docker ps
workshop/content/docs/setup/index.md
Outdated
[Docker Compose](https://docs.docker.com/compose/) is an addition to Docker to facilitate | ||
the orchestration of one or more Docker 'Containers' (a Container is a running instance of a Docker Image) | ||
[Docker Compose](https://docs.docker.com/compose/) is an addition to docker to facilitate | ||
the orchestration (configuration) of one or more Docker 'Containers' (a Container is a running instance of a docker 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.
same here 'Docker Image` (and Container) all capitalized.
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.
Image and Container not sure, they are not names; check at https://en.wikipedia.org/wiki/Docker_(software), they use image and container
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.
See my inline comments, mainly to use capitals..
Ok, in the Setup section, indeed may download the config file from the pygeoapi GH repo, for demonstrating how to mount a config file, and use the '-e' option.
4b78e61
to
23e1714
Compare
updated to use |
Indeed product names like Docker Compose capitalised, but artefacts like 'image' and 'container' lowercase. |
we may have to fix Compose then (other pr) |
i aligned the layout with other pages and updated some of the text/snippets
@justb4, what do you think