#90 #279 branded clusters [WIP] #407

dan-ash · 2020-12-12T20:33:06Z

This is a PR based on #90 add custom AWS clusters (see examples/aws.png for reference)

I would love to here feedback before I will consider this to be ready.

…com/givanovexpe/diagrams into givanovexpe-feature/add_icons_to_cluster_labels

…0-279-branded-clusters

wolfspyre · 2020-12-18T01:45:24Z

COOL!
This looks neat aesthetically.

@mingrammer What do ya think?

diagrams/__init__.py

bkmeneguello

This is amazing, and it's working very well. I'm integrating "Diagrams" into my company (>2k workers) just because of this implementation.

dan-ash · 2020-12-24T06:35:59Z

@bkmeneguello @wolfspyre thank you for the feedback I added Azure clusters resources as well.
Please note that #404 adding VMScaleSet to azure and I added this Cluster here(commented for now)

bkmeneguello · 2020-12-24T11:57:32Z

I'm trying an idea here to merge the functionality of the cluster with the nodes, so we could get rid of the cluster class and any node could be used with "with" and make use of this icon you introduced. WDYT?

dan-ash · 2020-12-24T11:59:00Z

@bkmeneguello I like the idea!
although I think the use case is rather confined only for a set of resources

gabriel-tessier · 2020-12-27T11:55:40Z

@dan-ash
Cool!! +1 and examples are really good!

I checked your branch on my local and had 2 questions and one comment?

I was able to run autogen.sh without errors all the cluster files get format changes (fixing PEP8 E302), I just wonder what are "# ftmt: on / off" for as contents is not changed with or without this comments?
There's already examples for the website in docs/getting-started/examples.md can you consider using this directory instead of the examples directory you added?

The library have a logic, images files are added in a directory structure and a script generate py files and api doc. With your changes you write directly cluster.py files in the diagrams directory which is not the original way to go. I think that it's better to keep the original logic of the library.
Also I don't get the logic of the cluster file as for me it's just an example of how you can implement and define your cluster, maybe only example is enough. For sure having AvailabilityZone or PrivateSubnet already defined can help but it's still just an example for me.

dan-ash · 2020-12-27T15:58:26Z

Hey @gabriel-tessier
thanks for the feedback, I'll try to address your points

the cluster.py was added manually as I didn't want to write the automation to auto generate it.
I missed that, I will move it there
I didn't want at this stage to modify the Cluster class and allow specific arguments to be passed for _default_graph_attrs so I duplicated the code from it https://github.com/mingrammer/diagrams/blob/master/diagrams/__init__.py#L198-L208 and that is where the #ftmt comments came from. (this is 1 of the reason this PR is still draft
The main change to the Cluster class was introduced in https://github.com/mingrammer/diagrams/pull/90/files I only used it and added the basic Clusters for AWS & Azure as I find them the most useful.

How would you go with auto generate the cluster.py?
One way will be something like:

create a cluster directory in the resources directory and have it treated as a special case (not generate Nodes)
Duplicate the images from the other directories to that directories

But this doesn't feel right to me

gabriel-tessier · 2020-12-28T08:56:21Z

@dan-ash

I think the cluster as you defined them are just example on how they can be implemented, let me try to explain my thinking.
Let's take for example class Region you defined it with color: #AEB6BE and dashed... you choose this design and color by your own not based on how AWS define them.
By comparison the nodes that are defined in diagrams are the official icons of the providers. I hope the comparison make sense and I'm sorry if my explanation is not clear.

Maybe for the simplicity of the PR it's better to go step by step and first focus on adding the icon by updating the Cluster class.
@mingrammer WDYT

If we focus only on the changes on cluster to add the icon, here one small remark:

I think the following line you can skip the assignation to self._icon_label, and assign directly to self.dot.graph_attr["label"] it will make the code more light and prevent confusion as _icon_label is not used in the class and even not defined as attributs.
So this line:

self._icon_label = '<<TABLE border="0"><TR><TD fixedsize="true" width="' + str(self._icon_size) +'" height="' + str(self._icon_size) +'"><IMG SRC="' + _node._load_icon() + '"></IMG></TD><TD>' + self.label + '</TD></TR></TABLE>>'
                self.dot.graph_attr["label"] = self._icon_label

will become:

self.dot.graph_attr["label"] = '<<TABLE border="0"><TR><TD fixedsize="true" width="' + str(self._icon_size) +'" height="' + str(self._icon_size) +'"><IMG SRC="' + _node._load_icon() + '"></IMG></TD><TD>' + self.label + '</TD></TR></TABLE>>'

And also check that icon and icon exist before assign the icon.
So this line:
if self._icon:

will become:
if self._icon and self._icon_size:

And as example you can provide the cluster you defined:

from diagrams import Diagram, Edge, Cluster
from diagrams.aws.compute import EC2, ApplicationAutoScaling
from diagrams.aws.network import ELB, VPC, PrivateSubnet, PublicSubnet
from diagrams.onprem.compute import Server
from diagrams.onprem.container import Docker


class Region(Cluster):
  _default_graph_attrs = {
      "shape": "box",
      "style": "dotted",
      "labeljust": "l",
      "pencolor": "#AEB6BE",
      "fontname": "Sans-Serif",
      "fontsize": "12",
  }


class AvailabilityZone(Cluster):
    # fmt: off
    _default_graph_attrs = {
        "shape": "box",
        "style": "dashed",
        "labeljust": "l",
        "pencolor": "#27a0ff",
        "fontname": "sans-serif",
        "fontsize": "12",
    }
    # fmt: on


class VirtualPrivateCloud(Cluster):
    # fmt: off
    _default_graph_attrs = {
        "shape": "box",
        "style": "",
        "labeljust": "l",
        "pencolor": "#00D110",
        "fontname": "sans-serif",
        "fontsize": "12",
    }
    # fmt: on
    _icon = VPC


class PrivateSubnet(Cluster):
    # fmt: off
    _default_graph_attrs = {
        "shape": "box",
        "style": "",
        "labeljust": "l",
        "pencolor": "#329CFF",
        "fontname": "sans-serif",
        "fontsize": "12",
    }
    # fmt: on
    _icon = PrivateSubnet


class PublicSubnet(Cluster):
    # fmt: off
    _default_graph_attrs = {
        "shape": "box",
        "style": "",
        "labeljust": "l",
        "pencolor": "#00D110",
        "fontname": "sans-serif",
        "fontsize": "12",
    }
    # fmt: on
    _icon = PublicSubnet


class SecurityGroup(Cluster):
    # fmt: off
    _default_graph_attrs = {
        "shape": "box",
        "style": "dashed",
        "labeljust": "l",
        "pencolor": "#FF361E",
        "fontname": "Sans-Serif",
        "fontsize": "12",
    }
    # fmt: on


class AutoScalling(Cluster):
    # fmt: off
    _default_graph_attrs = {
        "shape": "box",
        "style": "dashed",
        "labeljust": "l",
        "pencolor": "#FF7D1E",
        "fontname": "Sans-Serif",
        "fontsize": "12",
    }
    # fmt: on
    _icon = ApplicationAutoScaling


class EC2Contents(Cluster):
    # fmt: off
    _default_graph_attrs = {
        "shape": "box",
        "style": "",
        "labeljust": "l",
        "pencolor": "#FFB432",
        "fontname": "Sans-Serif",
        "fontsize": "12",
    }
    # fmt: on
    _icon = EC2


class ServerContents(Cluster):
    # fmt: off
    _default_graph_attrs = {
        "shape": "box",
        "style": "rounded,dotted",
        "labeljust": "l",
        "pencolor": "#A0A0A0",
        "fontname": "Sans-Serif",
        "fontsize": "12",
    }
    # fmt: on
    _icon = Server


with Diagram(name="", direction="TB", filename="aws"):
    with Cluster("AWS", graph_attr={"fontsize": "15"}): # overwrite attributes for the default cluster
        with Region("eu-west-1", graph_attr={"pencolor": "#60193C", "bgcolor": "#E587B5"}): # one cluster defined but with overwritten attributes
            with AvailabilityZone("eu-west-1a"):
                with VirtualPrivateCloud(""):
                    with PrivateSubnet("Private"):
                        with SecurityGroup("web sg"):
                            with AutoScalling(""):
                                with EC2Contents("A"):
                                    d1 = Docker("Container")
                                with ServerContents("A1"):
                                    d2 = Docker("Container")

                    with PublicSubnet("Public"):
                        with SecurityGroup("elb sg"):
                            lb = ELB()

    lb >> Edge(forward=True, reverse=True) >> d1
    lb >> Edge(forward=True, reverse=True) >> d2

bkmeneguello · 2020-12-28T14:48:57Z

@gabriel-tessier

I did these changes to @dan-ash PR to allow any Node to be treated as a Cluster by adding a cluster=True parameter to the constructor when using a with block: https://github.com/dan-ash/diagrams/pull/2/files.
I think this could be a solution and allow people to create more elaborated diagrams, with nodes being other things like comments, routes, tags, inside any other node.
Examples:

with VPC(cluster=True):
    Comment("Some useful comment")
    with EC2(cluster=True):
        t1 = Tag()
        u1 = User()
        with Directory(cluster=True):
            with ConfigFile(cluster=True):
                c1 = Config()
                c2 = Config()

with RouteTable(cluster=True):
    r1 = Route()
    r2 = Route()

Ideally, this cluster=True should not exist. I'm trying to delay the node registering into the dot graph to the parent's __end__ command. This way it could be possible to detect if the node is standalone or is used as a Cluster.

What do you think?

dan-ash · 2020-12-28T19:19:23Z

@bkmeneguello awesome work!
I think it is a much better solution!

bkmeneguello · 2020-12-28T20:01:14Z

@dan-ash I've updated the PR, feel free to merge with your branch and follow @gabriel-tessier advice.

gabriel-tessier · 2020-12-29T03:17:21Z

@bkmeneguello
I left a comment on your PR.

@dan-ash
I think it's better to make only the icon change first so bkmeneguello can push a PR with his changes.

Anyway my comment are just advice, need to wait that mingrammer review the changes to merge them in master.
Make the changes smaller can help him to merge more quickly.

… class

bkmeneguello · 2020-12-29T18:46:23Z

@gabriel-tessier

I've updated the related PR. I know is not the "minimum" set of changes, but I think are the changes that make sense to be together.
The state I left the project, the Node has become a replacement for the Custer and the former has become a subclass of the first.
Also now is possible to have empty Clusters (that will behave as Nodes) and Edges to Clusters.

Node as cluster

dan-ash · 2020-12-30T06:43:58Z

@bkmeneguello I merged your changes to my branch, I will remove some of my changes like @gabriel-tessier suggested & will fix the failed tests

I will have some time during the weekend, really AMAZING work!

wolfspyre · 2020-12-30T18:06:03Z

This is awesome stuff, y’all! @mingrammer - What do you think about this? What would need to happen here to merge? @dan-ash , @bkmeneguello <https://github.com/bkmeneguello> does this need a new major/minor rev? breaking changes?

…

On Dec 30, 2020, at 12:44 AM, dan-ash ***@***.***> wrote: @bkmeneguello <https://github.com/bkmeneguello> I merged your changes to my branch, I will remove some of my changes like @gabriel-tessier <https://github.com/gabriel-tessier> suggested & will fix the failed tests I will have some time during the weekend, really AMAZING work! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#407 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM2KE7BZFKL4N4YNZUTJF3SXLD3ZANCNFSM4UYTKABQ>.

bkmeneguello · 2020-12-30T18:23:02Z

@wolfspyre

I took care to not mess with the public API and to keep the same behavior as before, but I cannot guarantee. It would be awesome if someone could run this version over as many as possible diagrams and review the API changes to ensure it is the same.
I think a minor version increase is a minimum, but, eventually, if @mingrammer wishes, We could break the compatibility, remove the Cluster (or make it an Alias of Node) and bump the major. (... I think even this will keep the compatibility with the current version ...)

dan-ash · 2021-01-10T05:21:13Z

Sorry friends, had some unexpected issues, hope to get to it this week.

dan-ash · 2021-01-28T18:17:30Z

Hey, I'm closing this in favour of #439

seema1711 · 2021-02-20T08:07:54Z

@gabriel-tessier
I tried this #407 (comment).
But it's not displaying the icons.

gabriel-tessier · 2021-02-22T05:39:38Z

@seema1711
If you copied the code and apply it on the last stable version it's normal that it's no working as the changes are not in the last version.
If you copied the code with the source code of the fork, I'm sorry I can 't help here as the PR is a Work In Progress and had several commit after I tried my example (also this PR is closed).
Even the new PR linked here (#437) look not working correctly.
If you only want to display icons you can always copy the original PR #90
and make you fork you can easily do it with docker, all the instructions are in the doc: https://github.com/mingrammer/diagrams/blob/master/DEVELOPMENT.md

Hope it help.

seema1711 · 2021-02-22T16:52:01Z

Thanks!

Georgi Ivanov and others added 4 commits April 4, 2020 01:15

ability to add icons to the cluster label

45ffea9

Merge branch 'feature/add_icons_to_cluster_labels' of https://github.…

b9f53a8

…com/givanovexpe/diagrams into givanovexpe-feature/add_icons_to_cluster_labels

Merge branch 'givanovexpe-feature/add_icons_to_cluster_labels' into 9…

c93af91

…0-279-branded-clusters

AWS Branded clusters

62a2441

mingrammer added comp/cluster Issue of cluster component kind/feature New feature or request status/need-to-review Need to review labels Dec 18, 2020

bkmeneguello reviewed Dec 23, 2020

View reviewed changes

diagrams/__init__.py Outdated Show resolved Hide resolved

bkmeneguello approved these changes Dec 23, 2020

View reviewed changes

dan-ash added 2 commits December 24, 2020 08:09

CR changes

676db8e

Azure base clusters

d6f73ec

bkmeneguello added 2 commits December 28, 2020 16:58

Allow nodes to be user as cluster

dc33010

Allow node to cluster edges

496a69c

bkmeneguello added 3 commits December 29, 2020 10:27

Allow Node as Cluster with same label

0115211

Allow for empty "Node as Cluster" to render as Node

fc9ac5b

Convert the "Cluster" class to a "Node" and extract commons to a base…

bf6a41a

… class

bkmeneguello added 2 commits December 29, 2020 15:53

Allow Node icon to be modified

678587a

Fixed issue with nested Clusters and Cluster >> Node edges

91b5798

bkmeneguello and others added 3 commits December 29, 2020 18:19

Escape HTML entities of Cluster label

417047d

Better handling of multiline Cluster labels

070308c

Merge pull request #2 from bkmeneguello/node-as-cluster

88fb1f4

Node as cluster

This was referenced Jan 5, 2021

Allow any Node to behave as a Cluster #34

Open

Enable cluster branding #279

Open

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

Open

clayms mentioned this pull request Jan 11, 2021

Edge labels not placed with line #433

Open

bkmeneguello mentioned this pull request Jan 19, 2021

Node as cluster #438

Open

dan-ash mentioned this pull request Jan 23, 2021

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

Open

dan-ash closed this Jan 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#90 #279 branded clusters [WIP] #407

#90 #279 branded clusters [WIP] #407

dan-ash commented Dec 12, 2020

wolfspyre commented Dec 18, 2020

bkmeneguello left a comment

dan-ash commented Dec 24, 2020

bkmeneguello commented Dec 24, 2020 via email •

edited

Loading

dan-ash commented Dec 24, 2020 •

edited

Loading

gabriel-tessier commented Dec 27, 2020

dan-ash commented Dec 27, 2020

gabriel-tessier commented Dec 28, 2020

bkmeneguello commented Dec 28, 2020

dan-ash commented Dec 28, 2020

bkmeneguello commented Dec 28, 2020

gabriel-tessier commented Dec 29, 2020

bkmeneguello commented Dec 29, 2020

dan-ash commented Dec 30, 2020

wolfspyre commented Dec 30, 2020 via email

bkmeneguello commented Dec 30, 2020

dan-ash commented Jan 10, 2021

dan-ash commented Jan 28, 2021

seema1711 commented Feb 20, 2021

gabriel-tessier commented Feb 22, 2021

seema1711 commented Feb 22, 2021

#90 #279 branded clusters [WIP] #407

#90 #279 branded clusters [WIP] #407

Conversation

dan-ash commented Dec 12, 2020

wolfspyre commented Dec 18, 2020

bkmeneguello left a comment

Choose a reason for hiding this comment

dan-ash commented Dec 24, 2020

bkmeneguello commented Dec 24, 2020 via email • edited Loading

dan-ash commented Dec 24, 2020 • edited Loading

gabriel-tessier commented Dec 27, 2020

dan-ash commented Dec 27, 2020

gabriel-tessier commented Dec 28, 2020

bkmeneguello commented Dec 28, 2020

dan-ash commented Dec 28, 2020

bkmeneguello commented Dec 28, 2020

gabriel-tessier commented Dec 29, 2020

bkmeneguello commented Dec 29, 2020

dan-ash commented Dec 30, 2020

wolfspyre commented Dec 30, 2020 via email

bkmeneguello commented Dec 30, 2020

dan-ash commented Jan 10, 2021

dan-ash commented Jan 28, 2021

seema1711 commented Feb 20, 2021

gabriel-tessier commented Feb 22, 2021

seema1711 commented Feb 22, 2021

bkmeneguello commented Dec 24, 2020 via email •

edited

Loading

dan-ash commented Dec 24, 2020 •

edited

Loading