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

Improve documentation templates #618

Merged
merged 1 commit into from
Jan 24, 2024
Merged

Conversation

hanefi
Copy link
Contributor

@hanefi hanefi commented Jan 4, 2024

  • Adds CI to check that the documentation templates are up-to-date.
  • Add a new help command to the documentation templates. This command prints the help message for all the sub-commands of pgcopydb in a single file. This is useful for users who want to see all the sub-commands in one place.
  • Remove dependency on installing the binary before updating the documentation templates. This allows users to update the documentation templates without installing the binary. One can set the PGCOPYDB environment variable to point to the binary to use for updating the documentation templates.
  • Fix a casing issue in command descriptions. The first letter of the first word in the description of each command should be capitalized.

Dockerfile Outdated
Comment on lines 52 to 53
# Now the "docs-template-testing" image, to check if the docs are up to date
FROM --platform=${TARGETPLATFORM} build as docs-template-testing
Copy link
Owner

Choose a reason for hiding this comment

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

Does this means we run the docs-template-testing bits each time we want to run a test case (using e.g. make tests/pagila) on our local systems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Docker is clever enough to skip build stages that are not used. All thanks to the default backend (since last February) BuildKit for Docker.

See the list at the beginning of the page for BuildKit

Copy link
Owner

Choose a reason for hiding this comment

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

I see. I think I would still prefer it if we had a separate Dockerfile for the docs, does it make sense to split things that way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Docker does a poor job managing dependencies across multiple Dockerfiles. I created a new Dockerfile.tests file with a copy of our build stage so that we can hit docker build cache.

Makefile Outdated Show resolved Hide resolved
docs/update-help-messages.sh Outdated Show resolved Hide resolved
@dimitri dimitri added the documentation Improvements or additions to documentation label Jan 5, 2024
@hanefi hanefi requested a review from dimitri January 8, 2024 13:51
Comment on lines 78 to 81
- name: Check that docs are up to date
run: |
docker build --file=Dockerfile.tests --tag tests . && \
docker run tests
Copy link
Owner

Choose a reason for hiding this comment

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

We now have two main Dockerfile, the one that we use to run tests and the one that we use to build documentation. Why is the documentation one named Dockerfile.tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently it does not build docs. It only checks (or tests) if the documentation templates are up to date.

Is there any other name you'd prefer? Let me move this file to ci/Dockerfile if it will make it clear that it is not a main Dockerfile.

Copy link
Owner

Choose a reason for hiding this comment

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

Well I think I would prefer ci/Dockerfile.docs (the move to ci/ is nice). And if the contents have to contain a copy/paste version of the main Dockerfile, can we script that, use a template/include mechanism (only in the ci/ version, leave the main Dockerfile alone/unchanged)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the file to ci/Dockerfile.docs.template. One can run the following command to generate the final Dockerfile that is ignored in the git worktree:

cat Dockerfile ci/Dockerfile.docs.template > ci/Dockerfile.docs

I do not expect users to actually use this dockerfile in any way. I think make update-docs should be the command to run on development environments.

- Adds CI to check that the documentation templates are up-to-date.
- Add a new `help` command to the documentation templates. This command
  prints the help message for all the sub-commands of `pgcopydb` in a
  single file. This is useful for users who want to see all the
  sub-commands in one place.
- Remove dependency on installing the binary before updating the
  documentation templates. This allows users to update the documentation
  templates without installing the binary. One can set the `PGCOPYDB`
  environment variable to point to the binary to use for updating the
  documentation templates.
- Fix a casing issue in command descriptions. The first letter of the
  first word in the description of each command should be capitalized.
@hanefi hanefi requested a review from dimitri January 24, 2024 13:03
@dimitri dimitri merged commit 88a34e8 into dimitri:main Jan 24, 2024
18 checks passed
@hanefi hanefi deleted the docs-improvements branch January 25, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants