Integrate AWS-PlantUML into PlantUML

arnaudroques commented 7 years ago

Many thanks for this set of sprites and goodies. It's really super-useful.

Actually, it's so useful that we would like to include these to the standard distribution of PlantUML. Following the C convention for "C standard library" (see https://en.wikipedia.org/wiki/C_standard_library ), users would be able to use something like:

!include <aws/common.puml>
!include <aws/ApplicationServices/AmazonAPIGateway/AmazonAPIGateway.puml>
!include <aws/Compute/AWSLambda/AWSLambda.puml>
!include <aws/Compute/AWSLambda/LambdaFunction/LambdaFunction.puml>
!include <aws/Database/AmazonDynamoDB/AmazonDynamoDB.puml>
!include <aws/Database/AmazonDynamoDB/table/table.puml>
!include <aws/General/AWScloud/AWScloud.puml>
!include <aws/General/client/client.puml>
!include <aws/SDKs/JavaScript/JavaScript.puml>
!include <aws/General/user/user.puml>
!include <aws/Storage/AmazonS3/AmazonS3.puml>
!include <aws/Storage/AmazonS3/bucket/bucket.puml>

This would simply work out-of-the-box, without any other requirement. The only drawback is that it will increase plantuml.jar size of 2.5 Mo.

1) What do you think about it ? 2) Would you agree that we distribute your "dist" folder into our product ? 3) If you wish, we can also cite you in

@startuml
authors
@enduml

4) In our mind, the documentation for AWS-PlantUML would remain in this repository, and this repository will be the "official" version for AWS-PlantUML. We will just copy the "dist" folder. So that you will be able to continue to improve it.

Does it sound good to you ?

Ducatel commented 7 years ago

This is a great idea 👍

milo-minderbinder commented 7 years ago

Responding on mobile so apologies in advance for any spelling errors. First off - thank you for an indispensable diagramming tool, from someone in AppSec who has no free-form diagramming skills, yet has to update/amend diagrams constantly! I'm super flattered that you, and the other users of AWS-PlantUML, have shown interest in this project, and I'm glad you've found it useful. Even more so that you consider it useful enough to integrate into the core PlantUML project - so thanks!

I'm certainly not averse to the idea, and from its inception this project has been something I've put up on GitHub for anyone to use in whatever way is useful to them. In that spirit, and given that this project would not exist without PlantUML in the first place, if the PlantUML project wants to include AWS-PlantUML's dist with the standard distribution, I'd consider it an honor to contribute to the project in that way (a credit in the @authors would be great! 🙂), and leave that decision up to you with my permission.

I'll be brief(ish) here until I can get in front of my computer to provide more color and context, but I do have some additional thoughts regarding the size impact to the jar and how that could be reduced, how to ensure the included dist meets the majority of use cases while still enabling customization and flexibility for alternative use cases/preferences, etc.. I'll also share a more generic implementation I haven't pushed to a public repo yet, which allows users to similarly create configurable sprites and macros for other icon sets (e.g for other platforms, like Azure or GCP, or for miscellaneous architecural components like Jenkins, nginx, whatever).

arnaudroques commented 7 years ago

Ok, so we've build a new beta https://www.dropbox.com/s/koo42q3d9gxw288/plantuml.jar?dl=0 With this beta, you can have:

@startuml
!include <aws/common.puml>
!include <aws/Storage/AmazonS3/AmazonS3.puml>
!include <aws/Storage/AmazonS3/bucket/bucket.puml>

AMAZONS3(s3_internal)
AMAZONS3(s3_partner,"Vendor's S3")
s3_internal <-- s3_partner
@enduml

Few remarks:

We have also modified the @startuml/authors/@enduml. Please tell us if it's ok for you !
You can open the .jar file with any zip editor. There is a new "stdlib" folder. Include are searched in this folder. You can even patch our beta.
We have named your library "aws" but maybe a better name is possible
We've noticed that you have @startuml and @enduml in every .puml file. Actually this is not really need, so maybe you can remove them in all files
We are concerned about case: we would like the !include to be case insensitive. Unfortunately, this means that we should rename all your files in lowercase. Let me explain why : if in "stdlib" filepath is completely lowercased (this is the case for the test files foo/example1.puml we've put in the beta), then the following include are working: !include <foo/example1.puml> !include <Foo/Example1.puml> !include <FOO/example1.puml>

The .puml extension is optional and the following is also working:


@startuml
!include <aws/common>
!include <aws/Storage/AmazonS3/AmazonS3>
!include <aws/Storage/AmazonS3/bucket/bucket>

AMAZONS3(s3_internal) AMAZONS3(s3_partner,"Vendor's S3") s3_internal <-- s3_partner @enduml


This is a first draft, so we are open to suggestions!
It's really nice that you have gather so many goodies in AWS-PlantUML. I'm sure that other users will love that!

milo-minderbinder commented 7 years ago

@arnaudroques @Ducatel

We have also modified the @startuml/authors/@enduml. Please tell us if it's ok for you !

Thanks! Super minor, but if you could change to my real name (Chris Passarello), I'd prefer that

We've noticed that you have @startuml and @enduml in every .puml file. Actually this is not really need, so maybe you can remove them in all files

Fixed in latest release!

We are concerned about case: we would like the !include to be case insensitive. Unfortunately, this means that we should rename all your files in lowercase.

Fixed in latest release, also. I did notice that the case-insensitive path names don't work with the !includeurl directive, however. I can think of reasons why that behavior should stay and differ from !include, but it also makes the path/file names harder to parse for people who are using !includeurl since I've renamed all folders/files in lowercase and therefore made it harder pick out words than it was with title case. Any thoughts on how to get the best of both worlds?

The .puml extension is optional

To clarify, should I also remove the .puml extension from the files? or it's just optional when using !include?

arnaudroques commented 7 years ago

Thanks! Super minor, but if you could change to my real name (Chris Passarello), I'd prefer that

No problem, done in last beta http://beta.plantuml.net/plantuml.jar

To clarify, should I also remove the .puml extension from the files? or it's just optional when using !include?

No, do not remove the .puml extension from the file. Leave them XXX.puml. It's just optional when using !include.

Fixed in latest release, also. I did notice that the case-insensitive path names don't work with the !includeurl directive, however. I can think of reasons why that behavior should stay and differ from !include, but it also makes the path/file names harder to parse for people who are using !includeurl since I've renamed all folders/files in lowercase and therefore made it harder pick out words than it was with title case. Any thoughts on how to get the best of both worlds?

Yes, an idea... But I'm feeling uncomfortable with it because it means to restore your case-sensitive path name and file name :-( I'm really sorry about that, because I don't like the idea of undoing some work that has just been done.

Let's me explain why this change:

1) I agree with you: the repository and !includeurl is now harder to read, it was really better with case.

2) We have completely changed the way we store internally stdlib in the last beta ( http://beta.plantuml.net/plantuml.jar )

The real reason why we redesign this is that we were concerned by the size of plantuml.jar file which was inflating. Rather than storing each .puml of yours in plantuml.jar we decide to take advantage of the new brotli compression algorithm provided by Google ( https://github.com/google/brotli ) So in last beta, all .puml of each folder are packed together in an internal .rep file which is compressed using brotli.

So now, filename/filepath case is really not an issue anymore (because !include could now be smart enough to deal with case, and this could not previously be done because we were before relying on Java case sensitive mecanism of loading .puml from .jar file)

Note that this is completely transparent for end-user. There is also a new -extractstdlib flag so that people can have a look on source code of the library: java -jar plantuml.jar -extractstdlib

And the full stdlib that was using about 2500KB in last plantuml.jar has now decrease to about 770KB (plus 80KB for brotli decompressor code).

Thanks again for your library and sorry about this case confusion, I hope that reverting your change is not a big deal for you.

milo-minderbinder commented 7 years ago

@arnaudroques no worries - just reverted the pathname changes and related doc changes to master and the release/17-10-18 branches.

Incidentally, what is the ideal process for getting updates to AWS-PlantUML pushed to PlantUML's stdlib? Do you prefer just doing stdlib updates whenever you do a normal planned release (and only then), or would it be just as easy for me to create a PR to the PlantUML upstream with the generated .pumls whenever I release changes on my end? Alternatively, I can create a separate repo with just the contents of dist, so that it can be added as a git submodule on your end, if that's easier. Whichever you guys prefer, just let me know!

arnaudroques commented 7 years ago

Great! We've published a new beta with the last version of aws library. http://beta.plantuml.net/plantuml.jar

Good question about the process: we've just created a new repository https://github.com/plantuml/plantuml-stdlib that contains only the standard library.

This way, you can create PR to this repository whenever you want. The changes will be added in upcoming releases and betas.

milo-minderbinder / AWS-PlantUML

Integrate AWS-PlantUML into PlantUML #20