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

The include section of all "percy" diagrams looks wrong #93

Closed
kirchsth opened this issue Dec 12, 2020 · 3 comments · Fixed by #107
Closed

The include section of all "percy" diagrams looks wrong #93

kirchsth opened this issue Dec 12, 2020 · 3 comments · Fixed by #107
Milestone

Comments

@kirchsth
Copy link
Contributor

kirchsth commented Dec 12, 2020

all "percy" diagramms has a include section like

!include ./../C4_Container.puml
!include ./../C4_Context.puml
!include ./../C4.puml

This looks wrong for me.

  1. C4_Container.puml includes C4_Context.puml already, why is it repeated?
  2. If this pattern is used with "first" C4_Dynamic.puml and "last" C4.puml, then the automatic indexing is overwritten.

I think "percy" should include only the "first" *.puml and skip all following includes

BR Helmut

@adrianvlupu
Copy link
Member

They seem to be in the wrong order. I have no idea how percy does the includes, maybe @IOrlandoni has an opinion. I know that including diagrams locally is a bit different than including a URL.

@kirchsth
Copy link
Contributor Author

kirchsth commented Jan 8, 2021

@IOrlandoni, @adrianvlupu: based on the "false failure" build error in #107 PR I rechecked the current used include pattern in the test files. Independent of the error the additional include files can change the generated output file.
Eg. if it is used with dynamic diagrams the automatic counting of the relations is lost

@startuml
!include https://raw.githubusercontent.com/plantuml-stdlib/C4-PlantUML/master/C4_Dynamic.puml
' !include https://raw.githubusercontent.com/plantuml-stdlib/C4-PlantUML/master/C4_Container.puml
' !include https://raw.githubusercontent.com/plantuml-stdlib/C4-PlantUML/master/C4_Context.puml
' !include https://raw.githubusercontent.com/plantuml-stdlib/C4-PlantUML/master/C4.puml

LAYOUT_WITH_LEGEND()

ContainerDb(c4, "Database", "Relational Database Schema", "Stores user registration information, hashed authentication credentials, access logs, etc.")
Container(c1, "Single-Page Application", "JavaScript and Angular", "Provides all of the Internet banking functionality to customers via their web browser.")
Container_Boundary(b, "API Application") {
  Component(c3, "Security Component", "Spring Bean", "Provides functionality Related to signing in, changing passwords, etc.")
  Component(c2, "Sign In Controller", "Spring MVC Rest Controller", "Allows users to sign in to the Internet Banking System.")
}
Rel_R(c1, c2, "Submits credentials to", "JSON/HTTPS")
Rel(c2, c3, "Calls isAuthenticated() on")
Rel_R(c3, c4, "select * from users where username = ?", "JDBC")
@enduml

orig with counting

with additional includes no counting anymore

@startuml
!include https://raw.githubusercontent.com/plantuml-stdlib/C4-PlantUML/master/C4_Dynamic.puml
!include https://raw.githubusercontent.com/plantuml-stdlib/C4-PlantUML/master/C4_Container.puml
!include https://raw.githubusercontent.com/plantuml-stdlib/C4-PlantUML/master/C4_Context.puml
!include https://raw.githubusercontent.com/plantuml-stdlib/C4-PlantUML/master/C4.puml

LAYOUT_WITH_LEGEND()

ContainerDb(c4, "Database", "Relational Database Schema", "Stores user registration information, hashed authentication credentials, access logs, etc.")
Container(c1, "Single-Page Application", "JavaScript and Angular", "Provides all of the Internet banking functionality to customers via their web browser.")
Container_Boundary(b, "API Application") {
  Component(c3, "Security Component", "Spring Bean", "Provides functionality Related to signing in, changing passwords, etc.")
  Component(c2, "Sign In Controller", "Spring MVC Rest Controller", "Allows users to sign in to the Internet Banking System.")
}
Rel_R(c1, c2, "Submits credentials to", "JSON/HTTPS")
Rel(c2, c3, "Calls isAuthenticated() on")
Rel_R(c3, c4, "select * from users where username = ?", "JDBC")
@enduml

test file without counting

Therefore I think it is a better approach to add a switch in the C4_* files which loads the included files via url or local. And via an additional command line defintion it can be switched to the local version (if it is not already part of the "master" puml itself). This approach could be used independent of the test context too

e.g. C4_Dynamic.puml

!if %variable_exists("RELATIVE_INCLUDE")
  !include %get_variable_value("RELATIVE_INCLUDE")/C4_Component.puml
!else
  !include https://raw.githubusercontent.com/plantuml-stdlib/C4-PlantUML/master/C4_Component.puml
!endif
...

e.g. "percy\C4_Dynamic Diagram Sample - bigbankplc.puml"

@startuml
!if %variable_exists("RELATIVE_INCLUDE")
!include ./../C4_Dynamic.puml
!else
!include https://raw.githubusercontent.com/plantuml-stdlib/C4-PlantUML/master/C4_Dynamic.puml
!endif

local versions can be activated via
a) set variable in "master" puml (e.g. "percy\C4_Dynamic Diagram Sample - bigbankplc.puml")

@startuml
%set_variable_value("RELATIVE_INCLUDE", ".")
...

b) or via command line

java -jar plantuml.jar -DRELATIVE_INCLUDE="."  ...

kirchsth added a commit to kirchsth/C4-PlantUML that referenced this issue Jan 9, 2021
…line argument -DRELATIVE_INCLUDE="."

Update github workflow that local files are used
kirchsth added a commit to kirchsth/C4-PlantUML that referenced this issue Jan 9, 2021
…line argument -DRELATIVE_INCLUDE="." (2 - fix !if)
kirchsth added a commit to kirchsth/C4-PlantUML that referenced this issue Jan 9, 2021
…line argument -DRELATIVE_INCLUDE="."

Update github workflow that local files are used
@kirchsth
Copy link
Contributor Author

kirchsth commented Jan 9, 2021

the corresponding changes are part of PR #107 merged that workflow uses the correct local version during build that no "false failure" occurs anymore
#93: local file includes can be activated via command line argument -DRELATIVE_INCLUDE="."
Update github workflow that local files are used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
3 participants