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

all: add archview annotations #2964

Merged
merged 25 commits into from
Sep 10, 2019
Merged

all: add archview annotations #2964

merged 25 commits into from
Sep 10, 2019

Conversation

egonelbre
Copy link
Member

@egonelbre egonelbre commented Sep 5, 2019

Make codebase archview compatible. This adds annotations for satellite and storage node.

ArchView allows to visualize code architecture. However, it requires annotations to identify, which parts are important.

Makefile adds two targets make diagrams and make diagrams-graphml.

make diagrams requires GraphViz. As a result it outputs .svg files, this is the quick way to get a visualization.

make diagrams-graphml requires yEd or a similar tool. This will create .graphml files that can be opened in a graph editor and layout there.

The best configuration for yEd has been to use Hierarchical Layout, orientation left-to-right, edges octilinear, and layouting groups. Also you can manually group in yEd based on class or some other criteria.

Please describe the tests:

  • Test 1:
  • Test 2:

Please describe the performance impact:

Code Review Checklist (to be filled out by reviewer)

  • Does the PR describe what changes are being made?
  • Does the PR describe why the changes are being made?
  • Does the code follow our style guide?
  • Does the code follow our testing guide?
  • Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
  • Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
  • Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
  • Does any documentation need updating?
  • Do the database access patterns make sense?

@cla-bot cla-bot bot added the cla-signed label Sep 5, 2019
@egonelbre egonelbre added the WIP Work In Progress label Sep 5, 2019
@egonelbre egonelbre added Request Code Review Code review requested and removed WIP Work In Progress labels Sep 6, 2019
@egonelbre egonelbre marked this pull request as ready for review September 6, 2019 16:18
@egonelbre egonelbre requested a review from a team September 6, 2019 16:18
@ghost ghost requested review from mobyvb and zeebo and removed request for a team September 6, 2019 16:18
@egonelbre
Copy link
Member Author

Currently there still might be some things that are mis-classified.

@mobyvb
Copy link
Member

mobyvb commented Sep 6, 2019

Does the linter need to be updated around service/worker/chore/etc... comments?

@egonelbre
Copy link
Member Author

@mobyvb the linter doesn't seem to be complaining.

@mobyvb
Copy link
Member

mobyvb commented Sep 6, 2019

@mobyvb the linter doesn't seem to be complaining.

@egonelbre I guess I meant does the linter need to be updated so that it fails in the future if someone forgets to add an annotation to something that should have it?

@egonelbre
Copy link
Member Author

@mobyvb the issue is that the annotation should only be for relevant architecture types, not all of them. So it would be really hard to create an automatic decision. Otherwise I would have used that algorithm instead of annotations.

Copy link
Member

@ifraixedes ifraixedes left a comment

Choose a reason for hiding this comment

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

I dropped a comment and asked a few questions where I was not clear about the category of the type.

satellite/audit/pathcollector.go Show resolved Hide resolved
satellite/metainfo/loop.go Show resolved Hide resolved
storagenode/monitor/monitor.go Show resolved Hide resolved
@ifraixedes ifraixedes self-requested a review September 9, 2019 11:16
ifraixedes
ifraixedes previously approved these changes Sep 9, 2019
storagenode/pieces/cache.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@egonelbre egonelbre merged commit a801fab into master Sep 10, 2019
@egonelbre egonelbre deleted the ee/archview branch September 10, 2019 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants