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

dan-ash · 2021-01-23T21:13:57Z

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 · 2021-01-28T18:49:41Z

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 · 2021-01-28T18:55:50Z

Before the review, the approaches suggested by @bkmeneguello (#438 ) and @gabriel-tessier (#407 (comment)) 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 · 2021-01-28T19:08:56Z

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 ***@***.***> wrote: Before the review, the approach also suggested by @bkmeneguello <https://github.com/bkmeneguello> (#438 <#438> ) and @gabriel-tessier <https://github.com/gabriel-tessier> (#407 (comment) <#407 (comment)>) 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 <#439 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMGJMVH7O5Z44Z7CBKNZ6U3S4GXMPANCNFSM4WP7JAYA> .

bkmeneguello · 2021-02-01T13:55:21Z

@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 · 2021-02-22T11:25:25Z

@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:

#438 (comment)

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

scottstirling · 2021-03-07T04:09:38Z

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

dan-ash · 2021-03-07T05:21:51Z

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 ***@***.***> 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 <#439 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMGJMVCKF7BLZPSMWKPOOHDTCL4A5ANCNFSM4WP7JAYA> .

robert-matusewicz · 2021-05-24T12:03:49Z

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 · 2021-05-24T12:10:59Z

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 <#439 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMGJMVAW57OMLJYOJTYLH6LTPI6DJANCNFSM4WP7JAYA> .

robert-matusewicz · 2021-05-24T13:19:49Z

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 · 2021-06-10T14:57:57Z

@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 #438, also, which looks to be solving the same issues?

gabriel-tessier · 2021-06-12T04:29:46Z

@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 · 2021-08-03T09:59:30Z

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

dan-ash · 2021-08-07T10:41:32Z

@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 · 2022-02-01T09:16:00Z

@mingrammer is there a plan to get this merged?

strannik19 · 2022-06-22T15:44:04Z

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

FFengIll · 2022-10-05T10:57:12Z

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

diagrams/utils.py

diagrams/Cluster.py

docs/guides/cluster.md

mingrammer · 2022-11-04T08:48:26Z

Please resolve the conflicts from master :)

dan-ash · 2022-11-04T09:08:30Z

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

mingrammer · 2022-11-04T09:24:08Z

@dan-ash Sorry .. 😭

diagrams/utils.py

dan-ash · 2022-11-17T14:52:08Z

tests/test_diagram.py

@@ -47,12 +47,12 @@ def test_validate_curvestyle(self):
    def test_validate_outformat(self):


There was an issue with the tests, so I decided to modify the code to support the test, if that wasn't the test intention let me know and I will restore the previous

jobinjosem1 · 2022-12-08T13:25:28Z

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

jingai-db · 2022-12-17T23:16:14Z

Friendly ping!

Bakrog · 2023-01-10T12:58:00Z

Would be awesome to get this feature.

maor-lb · 2023-02-02T16:00:23Z

Certainly would be awesome

diagrams/__init__.py

LaTrissTitude · 2023-07-19T09:44:52Z

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

create a context abstract class Node as Cluster Adding documentation Revert node as cluster Fix multiple out format implementation & test CR Changes Group=Cluster

dan-ash · 2023-07-23T20:44:22Z

@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 · 2023-12-02T10:49:10Z

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

LaTrissTitude · 2023-12-02T11:12:19Z

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 <#439 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABHYJKDPWHUB2XODNYSI2T3YHMBTLAVCNFSM4WP7JAYKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBTG4YTCOBRHE3A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

chadfurman · 2023-12-02T13:57:31Z

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 > <#439 (comment)>, > 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 <#439 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE3CIXJC3UDJP2OXQ336O3YHMEKBAVCNFSM4WP7JAYKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBTG4YTEMRWHAZQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

gabriel-tessier · 2023-12-03T07:45:09Z

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.

maxrausch · 2024-01-20T05:07:19Z

Any updates ?

lucaspierru-convelio · 2024-02-27T10:58:34Z

Would be great to have this merged and released, especially for the ability to link Clusters! 👍

Karreg · 2024-02-28T20:21:34Z

It may help people here. I have, since my last comment, switched from diagrams to d2.

It may help you too, or not, depending on your use case.

LaTrissTitude · 2024-03-05T16:42:55Z

Oh that's nice; first time I hear about it and it does seems promising. Thanks for sharing !

…

On Wednesday, February 28th, 2024 at 21:21, Grégory Maitrallain ***@***.***> wrote: It may help people here. I have, since my last comment, switched from diagrams to d2. It may help you too, or not, depending on your use case. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

This was referenced Jan 27, 2021

Enable cluster branding #279

Open

ability to add icons to the cluster label #90

Open

#90 #279 branded clusters [WIP] #407

Closed

Node as cluster #438

Open

mingrammer added kind/feature New feature or request status/need-to-review Need to review labels Jan 28, 2021

chadfurman mentioned this pull request Jun 10, 2021

Is there a way to have arrows to and from Clusters #17

Open

dan-ash changed the title ~~Some reorg in order to help and allow other to contribute easier & Adding Branded Clusters support by enabling a Node to be used as a Cluster~~ Some reorg in order to help and allow others to contribute easier Aug 7, 2021

dan-ash changed the title ~~Some reorg in order to help and allow others to contribute easier~~ Some reorg in order to help and allow others to contribute more Aug 7, 2021

nvictor mentioned this pull request Apr 6, 2022

Connecting two cluster #670

Open

mingrammer reviewed Nov 4, 2022

View reviewed changes

diagrams/utils.py Show resolved Hide resolved

diagrams/Cluster.py Outdated Show resolved Hide resolved

docs/guides/cluster.md Outdated Show resolved Hide resolved

dan-ash force-pushed the master branch from cc4d9fd to 78cf336 Compare November 17, 2022 14:45

dan-ash commented Nov 17, 2022

View reviewed changes

dan-ash requested a review from mingrammer November 17, 2022 15:14

Viicos reviewed Jun 10, 2023

View reviewed changes

diagrams/__init__.py Outdated Show resolved Hide resolved

split classes into files

6201f5e

create a context abstract class Node as Cluster Adding documentation Revert node as cluster Fix multiple out format implementation & test CR Changes Group=Cluster

dan-ash force-pushed the master branch from fbc330a to 6201f5e Compare July 23, 2023 20:39

Anddd7 mentioned this pull request Dec 13, 2023

🚀 feature: add cluster icon Anddd7/diagrams-ext#1

Merged

		@@ -47,12 +47,12 @@ def test_validate_curvestyle(self):
		def test_validate_outformat(self):

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

Are you sure you want to change the base?

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

Conversation

dan-ash commented Jan 23, 2021 • edited Loading

mingrammer commented Jan 28, 2021

mingrammer commented Jan 28, 2021 • edited Loading

dan-ash commented Jan 28, 2021 via email

bkmeneguello commented Feb 1, 2021

gabriel-tessier commented Feb 22, 2021

scottstirling commented Mar 7, 2021

dan-ash commented Mar 7, 2021 via email

robert-matusewicz commented May 24, 2021

dan-ash commented May 24, 2021 via email

robert-matusewicz commented May 24, 2021

chadfurman commented Jun 10, 2021 • edited Loading

gabriel-tessier commented Jun 12, 2021 • edited Loading

DonDebonair commented Aug 3, 2021

dan-ash commented Aug 7, 2021

DonDebonair commented Feb 1, 2022

strannik19 commented Jun 22, 2022

FFengIll commented Oct 5, 2022 • edited Loading

mingrammer commented Nov 4, 2022

dan-ash commented Nov 4, 2022

mingrammer commented Nov 4, 2022

dan-ash Nov 17, 2022

Choose a reason for hiding this comment

jobinjosem1 commented Dec 8, 2022

jingai-db commented Dec 17, 2022

Bakrog commented Jan 10, 2023

maor-lb commented Feb 2, 2023

LaTrissTitude commented Jul 19, 2023

dan-ash commented Jul 23, 2023 • edited Loading

ivoras commented Dec 2, 2023

LaTrissTitude commented Dec 2, 2023 via email

chadfurman commented Dec 2, 2023 via email

gabriel-tessier commented Dec 3, 2023

maxrausch commented Jan 20, 2024

lucaspierru-convelio commented Feb 27, 2024

Karreg commented Feb 28, 2024

LaTrissTitude commented Mar 5, 2024 via email

dan-ash commented Jan 23, 2021 •

edited

Loading

mingrammer commented Jan 28, 2021 •

edited

Loading

chadfurman commented Jun 10, 2021 •

edited

Loading

gabriel-tessier commented Jun 12, 2021 •

edited

Loading

FFengIll commented Oct 5, 2022 •

edited

Loading

dan-ash commented Jul 23, 2023 •

edited

Loading