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

PlantUML diagrams #8712

Merged
merged 54 commits into from
Apr 6, 2021
Merged

Conversation

hjorthjort
Copy link
Contributor

Description

Sequence diagrams describing different aspects of staking, and a diagram describing the dependencies of keepers (seen from the object capability model).

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@amaury1093
Copy link
Contributor

amaury1093 commented Mar 15, 2021

Could we put those diagram files under a diagrams or plantuml subfolder in their respective folders? (#8712 (comment))

@hjorthjort
Copy link
Contributor Author

The diagrams have been moved to docs/uml. The advantage of doing that is that it simplifies the build process (over having them scattered over many different locations). I've also added the diagrams to sensible locations in the specs. I have used SVG images rather than ASCII, because I've found that the ASCII versions tend to overflow the layout of the documents. SVG becomes more compressed in the flow of the text, but should be straightforward to inspect closer.

Let me know what you think

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

@AmauryM this looks good to me now. Want to take another look.

@hjorthjort is this good to merge? I see you doing extra work here and there

@tac0turtle tac0turtle added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 6, 2021
@mergify mergify bot merged commit c41baa9 into cosmos:master Apr 6, 2021
@tac0turtle
Copy link
Member

oh whoops, this was premature. this broke docker builds, I will open a PR to fix it

@sahith-narahari
Copy link
Contributor

oh whoops, this was premature. this broke docker builds, I will open a PR to fix it

We should make this test required imo

@tac0turtle
Copy link
Member

oh whoops, this was premature. this broke docker builds, I will open a PR to fix it

We should make this test required imo

weird it's not.

@lovincyrus
Copy link
Contributor

this broke docker builds, I will open a PR to fix it

Just noticed this PR broke the docs build on master. Are you working on a fix? @marbar3778

@toschdev
Copy link

Thanks @lovincyrus
It would be great to have the master builds of docs back. All google links or doc links in the documentation are broken and me and others regularly hit 404s.

@tac0turtle
Copy link
Member

huh? ci is showing it builds correctly. https://github.com/cosmos/cosmos-sdk/tree/gh-pages has been pushed to, ci seems to be working. if it broke the build of the docs, then we need a way to check this. I havent stepped in the docs deployment world in a while. I can take a look tomorrow.

@@ -13,7 +13,7 @@
FROM golang:alpine AS build-env

# Install minimum necessary dependencies,
ENV PACKAGES curl make git libc-dev bash gcc linux-headers eudev-dev python3
ENV PACKAGES curl make git libc-dev bash gcc linux-headers eudev-dev python3 plantuml
Copy link
Member

Choose a reason for hiding this comment

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

@hjorthjort could you add a way to generate the plantuml? This is incorrect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marbar3778 Can you elaborate on "a way to generate the plantuml"? We need a plantuml binary to generate diagrams. By far the least hairy way would be to install it as a package from the standard package manager. Is it not available?

The other options are building it from source (hairier) or committing the official .jar directly, which would not stay up to date. The .jar option might be best if we can't install the package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option would be to do something like this: https://github.com/miy4/docker-plantuml/blob/master/Dockerfile

Copy link
Member

Choose a reason for hiding this comment

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

It seems you were trying to generate the uml files for useage in docs. This was failing because there is no uml binary. Is it okay we just dont generated the uml files into something that can be rendered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also possible to just generate the files locally (by anyone who has plantuml) and we simply commit the files. Not having them in the docs at all seems like a big step back, they are there to clarify things and there seemed to be a lot of positive feedback on having them.

But it's up to you what you think is the most hygenic approach.

Copy link
Member

Choose a reason for hiding this comment

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

we can generate them locally and push them. Could you do this? I dont see us changing these files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay. can you update your open PR to remove the plantuml from the Dockerfile and remove (instead of commenting out) the line for generating them.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be solved by #9124 (comment)

@okwme
Copy link
Contributor

okwme commented Apr 15, 2021

Thanks @lovincyrus
It would be great to have the master builds of docs back. All google links or doc links in the documentation are broken and me and others regularly hit 404s.

bump!

@lovincyrus lovincyrus mentioned this pull request Apr 15, 2021
12 tasks
@lovincyrus
Copy link
Contributor

Pushed a fix here → #9124

@lovincyrus
Copy link
Contributor

We should make this test required imo

#9125

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants