Some reorg in order to help and allow others to contribute more

[dan-ash]

Hey @mingrammer,

I'm aware that this PR is a big change but I believe it will help a lot for the maintainability of the project.

[mingrammer]

There is a lot of changes, so I need to review this feature carefully. (and there seems to be breaking changes) Let's take some time to review it and will get back.

[mingrammer]

Before the review, the approaches suggested by @bkmeneguello (#438 ) and @gabriel-tessier (https://github.com/mingrammer/diagrams/pull/407#issuecomment-751638239) also looks good to me personally. But I'm not sure which is the best for now before review it completely. (just comment)

[dan-ash]

I understand, I made sure to split the changes to logical commits, so may I suggest to review it by commit.

Thanks.

On Thu, Jan 28, 2021, 20:56 MinJae Kwon [email protected] wrote:

Before the review, the approach also suggested by @bkmeneguello https://github.com/bkmeneguello (#438 https://github.com/mingrammer/diagrams/pull/438 ) and @gabriel-tessier https://github.com/gabriel-tessier (#407 (comment) https://github.com/mingrammer/diagrams/pull/407#issuecomment-751638239) looks good to me personally. But I'm not sure which is best for now before review it completely. (just comment)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mingrammer/diagrams/pull/439#issuecomment-769300784, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMGJMVH7O5Z44Z7CBKNZ6U3S4GXMPANCNFSM4WP7JAYA .

[bkmeneguello]

@dan-ash

I tried your PR but it's not working like the other ones (specially #438):

from diagrams import Cluster, Diagram
from diagrams.aws.compute import ECS
from diagrams.aws.database import RDS, Aurora
from diagrams.aws.network import Route53, VPC

with Diagram("Simple Web Service with DB Cluster", show=True, filename="mysql"):
    dns = Route53("dns")
    web = ECS("service")

    with VPC('VPC'):
        # using cluster with an icon
        with ECS("DB ClusterA"):
            db_master1 = RDS("main")
            db_master1 - [RDS("replica1"), RDS("replica2")]
        # using the node
        with Aurora("DB ClusterA") as db2:
            db_master2 = RDS("main")
            db_master2 - [RDS("replica1"), RDS("replica2")]

        dns >> web >> db_master1
        # link to/from cluster
        dns >> web >> db2

[gabriel-tessier]

@dan-ash

I can confirm the problem raised bellow I just updated the name of the cluster to make it more clear:

from diagrams import Cluster, Diagram
from diagrams.aws.compute import ECS
from diagrams.aws.database import RDS, Aurora
from diagrams.aws.network import Route53, VPC

with Diagram("Simple Web Service with DB Cluster", show=True, filename="mysql"):
    dns = Route53("dns")
    web = ECS("service")

    with VPC('VPC'):
        # using cluster with an icon
        with ECS("DB ClusterA"):
            db_master1 = RDS("main")
            db_master1 - [RDS("replica1"), RDS("replica2")]
        # using the node
        with Aurora("DB ClusterB") as db2:
            db_master2 = RDS("main")
            db_master2 - [RDS("replica1"), RDS("replica2")]

        dns >> web >> db_master1
        # link to/from cluster
        dns >> web >> db2

which render like this on your branch:

and same code on bkmeneguello PR:

@bkmeneguello In your example you named both cluster "DB ClusterA" which make not clear what going on, but at least it's a good example...

On this PR there's also the same error about the color order reported on the pull/438:

https://github.com/mingrammer/diagrams/pull/438#issuecomment-783291179

But the "graph_attr" is working correctly in this PR.

[scottstirling]

Any update and/or anything anyone can do to help - test, review code?

[dan-ash]

I will try to fix the issue that was found this weekend, but have some personal issues.

On Sun, Mar 7, 2021, 06:09 Scott Stirling [email protected] wrote:

Any update and/or anything anyone can do to help - test, review code?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mingrammer/diagrams/pull/439#issuecomment-792198510, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMGJMVCKF7BLZPSMWKPOOHDTCL4A5ANCNFSM4WP7JAYA .

[robert-matusewicz]

Hey, I started using Diagrams and that PR looks like something that would be very helpful for me. Unfortunately, it looks outdated. @dan-ash and @mingrammer would it be a problem if I would raise a series of smaller PRs that would implement what was implemented in this PR?

[dan-ash]

I'll really be happy to see this go through, I was unable to push this myself due lack of time.

In order to not waste our time, how about we get @mingrammer https://github.com/mingrammer POV on this and have some discussion, so it wont get stuck again?

I have a bit more time now days so I can work on this also.

Thanks for raising this

On Mon, May 24, 2021, 15:04 Robert Matusewicz @.***> wrote:

Hey, I started using Diagrams and that PR looks like something that would be very helpful for me. Unfortunately, it looks outdated. @dan-ash https://github.com/dan-ash and @mingrammer https://github.com/mingrammer would it be a problem if I would raise a series of smaller PRs that would implement what was implemented in this PR?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mingrammer/diagrams/pull/439#issuecomment-846996635, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMGJMVAW57OMLJYOJTYLH6LTPI6DJANCNFSM4WP7JAYA .

[robert-matusewicz]

I think that it's a good idea. From my side, I would propose splitting that PR into refactoring one (so moving classes outside of init) and a functional one. The first should be fairly easy to review and then we can focus on a juicy, functional part!

[chadfurman]

@dan-ash might be good to split this PR into smaller entities -- pulling out the re-org by itself would be non-breaking, I imagine, and then that could get merged relatively quickly I'd hope (cc @mingrammer )

Do we know what the breaking changes are?

What are your thoughts on https://github.com/mingrammer/diagrams/pull/438, also, which looks to be solving the same issues?

[gabriel-tessier]

@chadfurman About #438 I already review and test with all the available example on the website (which took a lot of time). The fix I suggest solve the main bugs I found. So far nobody move.... I already built a pip package with this changes and a couple of other PR that are still pending. If you are interested I can share it but as it's not the official version you will not have any upgrade and I don't think I will take time to maintain (except for my needs) so you will use it at your own risk (so far nobody get hurt using it! 😀)

[DonDebonair]

Where are we on this one and/or #438 ? Would love to have this functionality!

[dan-ash]

@chadfurman @mingrammer I reverted the changes for the Nodes as clusters (aka branded clusters) hope that this will allow you with merging this change that is only related to ordering of the classes, I will then create a follow up PR with the changes that allows for using Nodes as Clusters.

[DonDebonair]

@mingrammer is there a plan to get this merged?

[strannik19]

just found this project recently. really need the ability to have edges between Clusters rather than just nodes.

[FFengIll]

It takes years (at least 2), and I am doubt there is something blocks all related PR merge. Hope to have these features.

[mingrammer]

Please resolve the conflicts from master :)

[dan-ash]

Hey, It has been a while since I created this PR but I will try to look over it and address the comments.

[mingrammer]

@dan-ash Sorry .. 😭

[jobinjosem1]

@mingrammer Could you please review and merge this one? This is a very cool feature

[jingai-db]

Friendly ping!

[Bakrog]

Would be awesome to get this feature.

[maor-lb]

Certainly would be awesome

[LaTrissTitude]

Is this review still ongoing ? The ability of linking clusters would be a game changer to link different scales together

[dan-ash]

@LaTrissTitude Thanks for raising it again, I got a bit frustrated with this project after so many years, but I had a bit of time, so I decided to update the PR. I hope @mingrammer or another maintainer will give it some attention. Over the years, this PR got a lot of similar responses, so I hope this time it will be reviewed and merged, and once that happens, I could contribute the PR that allows for Nodes to act as clusters, which will allow linking.

[ivoras]

Bump! For both reasons - branding clusters with service icons, and connecting clusters.

[LaTrissTitude]

To be fair, at this point, we might just have to use dan's fork while waiting for a merge 😅

Le sam. 2 déc. 2023, 11:49, Ivan Voras @.***> a écrit :

Bump! For both reasons - branding clusters with service icons, and connecting clusters.

— Reply to this email directly, view it on GitHub https://github.com/mingrammer/diagrams/pull/439#issuecomment-1837118196, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHYJKDPWHUB2XODNYSI2T3YHMBTLAVCNFSM4WP7JAYKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBTG4YTCOBRHE3A . You are receiving this because you were mentioned.Message ID: @.***>

[chadfurman]

I've basically just been using mermaid

On Sat, Dec 2, 2023, 06:12 Triss Jacquiot @.***> wrote:

To be fair, at this point, we might just have to use dan's fork while waiting for a merge 😅

Le sam. 2 déc. 2023, 11:49, Ivan Voras @.***> a écrit :

Bump! For both reasons - branding clusters with service icons, and connecting clusters.

— Reply to this email directly, view it on GitHub https://github.com/mingrammer/diagrams/pull/439#issuecomment-1837118196,

or unsubscribe < https://github.com/notifications/unsubscribe-auth/ABHYJKDPWHUB2XODNYSI2T3YHMBTLAVCNFSM4WP7JAYKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBTG4YTCOBRHE3A>

. You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/mingrammer/diagrams/pull/439#issuecomment-1837122683, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE3CIXJC3UDJP2OXQ336O3YHMEKBAVCNFSM4WP7JAYKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBTG4YTEMRWHAZQ . You are receiving this because you were mentioned.Message ID: @.***>

[gabriel-tessier]

I forked diagrams with cluster changes and the fixes I reported + other features, the lib is actively used by me for the web version and if you really want to use cluster as node give it a try.

https://github.com/diagrams-web/diagrams-xtd

I can push release or publish it on pypi if people use it.