-
Notifications
You must be signed in to change notification settings - Fork 49
Adding architecture documentation to DVO project #146
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
Conversation
Signed-off-by: npecka <zangoose107@gmail.com>
Signed-off-by: npecka <zangoose107@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #146 +/- ##
=======================================
Coverage 32.87% 32.87%
=======================================
Files 16 16
Lines 438 438
=======================================
Hits 144 144
Misses 274 274
Partials 20 20 |
|
I haven't yet found the opportunity to really dive into this with great detail to what I'd suggest to be added to or amended with this, but I do thank you again for getting this started! It's not easy to go from near-nothing to something with documentation for any project. The goal of this PR, in my opinion, should be to produce documentation that someone with minimal understanding of this project can look at, and come out with an excellent understanding of how the project works, and that person should be able to confidently explain the project to other people. I don't think this PR is accomplishing this at the moment. I feel this PR is significantly lacking in descriptive context on what the significant parts of DVO are actually doing and how they work. I think a good next step would be to accompany these 2 diagram you made with written descriptions of what each piece in the diagrams is doing, and what the arrows connecting them actually mean. |
|
I feel as though the diagrams in the current state are a bit busy and confusing. It looks like they're trying to combine all the following in a 1-2 diagrams:
I would love to see the 2 diagrams we have today split cleanly into 5 separate diagrams as described here. 😀 |
bkez322
left a comment
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 friendly suggestions above)
…ck from PR Signed-off-by: npecka <zangoose107@gmail.com>
I have pushed a change that introduces 5 new diagrams. They are still in rough draft form and will need to be revised to look more aesthetically pleasing. Looking for feedback on the information that goes along with them and what needs to be added / how you're looking for things to be worded. @bkez322 Please take a look and let me know your feedback once you have the chance. Thanks! |
Signed-off-by: npecka <zangoose107@gmail.com>
|
@npecka you've got an incorrect link: /images/DVO_Observability.drawio.png on line 32 of Architecture.md the filename has as space not an underscore "DVO Observability.drawio.png" so you'll either want change line 32 or remove it. It's referenced correctly later in the doc, so maybe you were looking to reference a different drawing here? |
Signed-off-by: npecka <zangoose107@gmail.com>
hlipsig
left a comment
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.
We can iterate over this with time, but it's better to have something.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hlipsig, npecka The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
bkez322
left a comment
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.
Sorry I'm late to comment, but LGTM!
Submitting architecture diagrams to DVO project to assist in understanding the overall picture for DVO