Skip to content

RFC: add architecture documentation inspired by the C4 model approach #1241

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

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

Conversation

LarsArtmann
Copy link

@LarsArtmann LarsArtmann commented May 8, 2025

What does this PR aim to accomplish?:

I was searching for a architecture diagram of pihole's internals. Since I was unable to find one, I created this.

Images

mermaid-diagram-level1
mermaid-diagram-level2
mermaid-diagram-level3
mermaid-diagram-DNS-Query-Flow
mermaid-diagram-Blocklist-Update-Process

How does this PR accomplish the above?:

The PR adds a new architecture.md file in the main folder.
The new file includes multiple mermaid sections that can be easily rendered into good looking diagrams.

Open Issue:

I did not manage to test my changes.
mkdocs build and mkdocs serve do not work for because of issue(s) with the template.

Screenshot 2025-05-08 at 18 38 14

Note

My understanding of the whole projects architecture is limited, it is very possible some of the diagrams have mistakes.
I do think it is a good start though. Input or changes are very welcome!


  • I have read the above and my PR is ready for review. Check this box to confirm

Copy link

netlify bot commented May 8, 2025

Deploy Preview for pihole-docs ready!

Name Link
🔨 Latest commit 067fff4
🔍 Latest deploy log https://app.netlify.com/projects/pihole-docs/deploys/6828f08bce77ac0008dc0027
😎 Deploy Preview https://deploy-preview-1241--pihole-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@LarsArtmann LarsArtmann marked this pull request as ready for review May 8, 2025 16:54
@LarsArtmann LarsArtmann requested a review from a team as a code owner May 8, 2025 16:54
@LarsArtmann
Copy link
Author

Looking at: the preview, to make this truly valuable, we would need to

  1. add a mermaid.js parser or
  2. use SVGs or PNGs instead.

I would tend to option 1.
Does anybody know enough about the building process to help with the implementation of mermaid.js parser support?

@LarsArtmann LarsArtmann changed the title docs: add architecture documentation inspired by the C4 model approach RFC: add architecture documentation inspired by the C4 model approach May 8, 2025
@yubiuser
Copy link
Member

yubiuser commented May 9, 2025

Does anybody know enough about the building process to help with the implementation of mermaid.js parser support?

Should be pretty simple: https://squidfunk.github.io/mkdocs-material/reference/diagrams/

@LarsArtmann LarsArtmann force-pushed the architecture-diagram branch from 9615a61 to 76a2a21 Compare May 16, 2025 16:08
@yubiuser
Copy link
Member

Before you invest more time in this PR: While it looks interesting at first to see the inner workings depicted, I'm not sure how much benefit users get from this. If I had no clue how Pi-hole works, the figures might not help much as they are very complex (as is Pi-hole).
My biggest concern with those images: they will need updates every time something changes. And we will likely forget about this.

@LarsArtmann
Copy link
Author

Before you invest more time in this PR: While it looks interesting at first to see the inner workings depicted, I'm not sure how much benefit users get from this. If I had no clue how Pi-hole works, the figures might not help much as they are very complex (as is Pi-hole).

These diagrams are aimed at potential contributors, not general users, to aid understanding of Pi-hole's internals.

My biggest concern with those images: they will need updates every time something changes. And we will likely forget about this.

A very reasonable concern.
We could just add a notice stating the diagrams may not be up-to-date, including the last update date, to manage maintenance expectations.

Place this under Contributing rather than General would provide better context.

Signed-off-by: Lars Artmann <git@lars.software>
Signed-off-by: Lars Artmann <git@lars.software>
Signed-off-by: Lars Artmann <git@lars.software>
@LarsArtmann LarsArtmann force-pushed the architecture-diagram branch from 76a2a21 to 067fff4 Compare May 17, 2025 20:24
@rdwebdesign
Copy link
Member

I think your suggestion is interesting, but the diagrams seems too complex. In my opinion, only users that already understand how Pi-hole works will understand the images.

We could just add a notice stating the diagrams may not be up-to-date

Sometimes we forget to update some things, but adding information we already know will be outdated is a bad practice.

@LarsArtmann
Copy link
Author

LarsArtmann commented May 19, 2025

Sometimes we forget to update some things, but adding information we already know will be outdated is a bad practice.

Good point. We don't need to include all of the charts I suggested. I can see how charts 1 and 2 could change, but the 3rd (flow) chart should remain fairly stable, shouldn't it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants