Added draft architecure file #276
Conversation
|
🧙 Sourcery has finished reviewing your pull request! Tips
|
WalkthroughThe recent change introduces a new PlantUML file, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Fabian1535 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
apps/misc/flyxc_arch.puml (1)
1-6: Remove unnecessary commented-out include.The include for
Cloud_Firewall_Rules.pumlis commented out. If it's not needed, consider removing it to keep the file clean.- '!include GCPPuml/Networking/Cloud_Firewall_Rules.puml
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/misc/flyxc_arch.puml (1 hunks)
Additional comments not posted (4)
apps/misc/flyxc_arch.puml (4)
16-24: LGTM!The AWS includes and the comment about needing other icons are appropriate.
26-38: LGTM!The elements are well-defined and appropriately named.
41-56: LGTM!The relations are well-defined and appropriately named.
58-58: LGTM!The
@endumlstatement is necessary to end the PlantUML diagram.
|
Thanks @Fabian1535 ! I think you can create a It would be nice to add a short description of the file and the way to generate an image in the top level Thanks! |
|
Created doc folder and added some info on CONTRIBUTING.md |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
CONTRIBUTING.md (2)
52-52: Fix the heading level.The heading level should be incremented properly. Change the heading level from
####to###.- #### Architecture Diagram + ### Architecture DiagramTools
Markdownlint
52-52: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
58-59: Fix the bare URL and grammatical issue.Avoid using bare URLs and correct the operating system's name.
- https://plantuml.com/starting + [PlantUML Starting Guide](https://plantuml.com/starting) - Press Alt + D to start PlantUML preview (option + D on MacOS). + Press Alt + D to start PlantUML preview (option + D on macOS).Tools
LanguageTool
[grammar] ~59-~59: The operating system from Apple is written “macOS”.
Context: ...o start PlantUML preview (option + D on MacOS).(MAC_OS)
Markdownlint
58-58: null
Bare URL used(MD034, no-bare-urls)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
doc/flyxc.pngis excluded by!**/*.png
Files selected for processing (2)
- CONTRIBUTING.md (1 hunks)
- doc/flyxc_arch.puml (1 hunks)
Additional context used
LanguageTool
CONTRIBUTING.md
[grammar] ~59-~59: The operating system from Apple is written “macOS”.
Context: ...o start PlantUML preview (option + D on MacOS).(MAC_OS)
Markdownlint
CONTRIBUTING.md
52-52: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
58-58: null
Bare URL used(MD034, no-bare-urls)
Additional comments not posted (3)
CONTRIBUTING.md (1)
54-57: LGTM!The instructions for using PlantUML are clear and helpful.
doc/flyxc_arch.puml (2)
2-24: LGTM!The inclusions of external libraries for GCP and AWS components are appropriate and necessary for the architecture diagram.
26-58: LGTM!The elements and relations in the architecture diagram are well-defined and provide a clear representation of the system's architecture.
doc/flyxc.png
Outdated
There was a problem hiding this comment.
Nice!
Few comments:
fxc-tiles is a local script to generate tiles and it stores the tiles in cloud storage.
- It should not be marked as "frontend"
- It should connect to storage instead of datastore - is it possible to annotate the connection with "airspace tiles" ?
- the frontend connects to storage to retrieve the tiles
Redis:
- Is connected to the server (fxc-server), not to the frontend directly
- The server reads from Redis
- There should be the fetcher too that retrieve the position from the live tracking platforms and push them to redis
Fetcher (to be added)
- Reads the user list from the datastore
- Collect the positions from the different live tracking platforms
- Store the positions in Redis
There was a problem hiding this comment.
Could you rename the png to flyxc_arch.png?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
doc/flyxc_arch.pngis excluded by!**/*.pngout/doc/flyxc_arch/flyxc_arch.pngis excluded by!**/*.png
Files selected for processing (2)
- CONTRIBUTING.md (1 hunks)
- doc/flyxc_arch.puml (1 hunks)
Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
Files skipped from review as they are similar to previous changes (1)
- doc/flyxc_arch.puml
There was a problem hiding this comment.
I think you can remove desktop/mobile and connect users to the frontend directly
- fxc-tiles has no direct connection to the frontend. It writes to the cloud storage and the frontend reads from there.
- I think you can split the cloud storage into 2 differents pieces (they are actually stored in different "buckets")
- the tiles (generated by fxc-tiles)
- the tracks (generated by both Cloud functions and fxc-server)
Otherwise this looks pretty good, should be ready to merge soon.
Thanks for your work on this!
|
I just learned that GH markdown actually support diagrams: https://github.blog/developer-skills/github/include-diagrams-markdown-files-mermaid/ Maybe something to explore... and then there is no need to install anything nor re-generate the image on change |
Added a PlantUML file with a draft architecture for flyxc
Summary by CodeRabbit