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

MVP implementation for Groups #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anton-kasperovich
Copy link

@anton-kasperovich anton-kasperovich commented May 4, 2023

By no means this is a proper implementations for Groups, but I would like to kick off discussion on how owners of the Azure-PlantUML would see it should be implemented correctly. The codebase which I have in PR does work (not the best UI and will be improved), but obviously making if statement to split "entity/group" is not great.

Could you please have a look and guide on how would you see this should be done to be accepted?

Screenshot 2023-05-04 at 12 15 24

P.S. 100% inspired by AWS PlantUML, kudos to @mcwarman

@Potherca
Copy link
Member

Potherca commented May 5, 2023

As co-maintainer I'll have to defer to @travisnielsen and/or @RicardoNiepel for any final say.

That said, I don't see any major downsides in adding support for grouping.

I do wonder if/where a definitive list is available of what constitutes a "group"?

As for the implementation, there are indeed some rough edges that need refining (for instance the use of positional parameter instead of named parameters) but those can easily be address by the regular review process.

For me, the major omission at this point is documentation that describes the usage of and motivation behind having "groups". What are they good for? When to use them? How to use them? Which groups should be used for what? How to add/create custom groups? Etc.

Obviously, backward compatibility is desirable. We might want to set up testing (similar to the Percy testing as currently done in C4-Plantuml).

@travisnielsen
Copy link
Contributor

I think of this as parent / child nesting, which is an important part of any cloud deployment diagram and, technically speaking, is already supported in PlantUML. Last year I had looked into ways to improve the layout this by adding overrides that simplify parent objects. This approach doesn't introduce the idea of a different object type for groups because so many Azure services could have parent containers (App Service Plan, Event Hub clusters, etc...). It also doesn't require changes to the sprite generation code. In any case, this is what I ended up with:

image

The tradeoff in my POC implementation is the need for 'null' values in parameters.

In any case, I can see the benefits of this approach in terms of clarity, but it comes with a need to define all the object types that could conceivably be a group. If the idea is to keep support limited to a handful of items like Subscriptions, Resource Groups, and VNETs, I suppose this would be a nice addition to Azure PlantUML.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants