search

Midnighter / structurizr-python

A Python 3 port of Simon Brown's Structurizr diagrams-as-code architecture description tool.
https://structurizr.com/
Apache License 2.0
66 stars 16 forks source link

Support free tier #73

Closed maximveksler closed 3 years ago

maximveksler commented 3 years ago

The code will fail on attempting to lock the workspace, a feature which is not supported on the free tier.

This PR introduces logic to cope with that by attempting to derive if the user is running on a free tier, in which case the failure should not be considered fatal.

Would appreciate help to test if it's working for the paid tier as well before merging.

Midnighter commented 3 years ago

Hi @maximveksler,

Thank you for the contribution. Maybe there are multiple approaches to take here:

  1. I had hoped that it was clear from the docs that using a context is not necessary at all. You can just call .get and .put which will not attempt to lock the workspace. So the docs clearly need improvement.
  2. @yt-ms previously suggested changing the way that a context is entered which should also make it more clear that the purpose of the context is to lock the workspace.

I favour a solution as proposed by @yt-ms that includes logic as you propose here. That means, ignoring the lock if on a free plan (with a warning) and only raising an exception if (un-)locking fails on a paid plan.

maximveksler commented 3 years ago

@Midnighter kudos on the implemention quality. A pleasure to read your code.

On the topic of free / paid -

I think that exception behaviour should be consistent between the plans.

I.e. warn on lock and unlock on the free. And error on lock fail and unlock failure on paid.

It's not super clear why the locking is required in the 1st if a merge strategy is implements, but that's a different topic.

What happens if you attempt to unlock an unlocked workspace? I believe that should not be an error as it's idempotent with no side effects.

Thoughts?

Midnighter commented 3 years ago

It's not super clear why the locking is required in the 1st if a merge strategy is implements, but that's a different topic.

As far as I'm aware, no merge strategy is implemented on the Structurizr API side but @simonbrowndotje and team are constantly adding new features so I might have missed that. Allowing multi-tenancy via something like CRDT would be a very exciting feature 😃 but it would probably mean starting over for the backend.

I have thought about implementing a diff on the client side. So whenever a user calls .put, it would internally get the remote workspace and then display a diff with maybe an interactive confirmation for overwriting the remote workspace. One could expand on this. However, the general strategy for architecture as code would be to have everything checked into version control so that you can always generate your workspace from scratch. That means, git should take care of conflicts for teamwork and thus there is no need on the API side to handle this.

The diff display plus confirmation is not a lot of work, though, so that could be implemented.

What happens if you attempt to unlock an unlocked workspace? I believe that should not be an error as it's idempotent with no side effects.

Indeed the response from the API is always success.

simonbrowndotje commented 3 years ago

It's not super clear why the locking is required in the 1st if a merge strategy is implements, but that's a different topic.

The workspace locking mechanism is a pessimistic approach to concurrency, and only needed for paid plans where workspaces can be shared using the role-based access mechanism. For free plans, there's no need to lock/unlock.

simonbrowndotje commented 3 years ago

As far as I'm aware, no merge strategy is implemented on the Structurizr API side

Correct, all workspace merging (of manual layout information) is done client-side ... it's a more flexible approach, and users can customise the merging algorithm if desired.

The diff display plus confirmation is not a lot of work, though, so that could be implemented.

I started work on a semantic workspace diff engine for the CLI and it's not particularly straightforward. 😀 I will resurrect this at some point though.

Midnighter commented 3 years ago

I started work on a semantic workspace diff engine for the CLI and it's not particularly straightforward. grinning I will resurrect this at some point though.

I was thinking of a cheap way where I would use an existing tool to just do a JSON diff of the serialized workspaces. Probably not always the most intelligible output but better than nothing 🤷🏼‍♂️ Do you see difficulties with that, too, @simonbrowndotje?

simonbrowndotje commented 3 years ago

I tried a number of JSON diff tools myself, and even integrated a couple as a basis for my diff engine - they're very noisy, and usually don't give good results when the ordering of elements changes between versions, etc.

yt-ms commented 3 years ago

I tried a number of JSON diff tools myself, and even integrated a couple as a basis for my diff engine - they're very noisy, and usually don't give good results when the ordering of elements changes between versions, etc.

I actually have an approach that I've been using on a small scale that seems to work well. I have some Python code built on top of this library that will output a workspace in C4 DSL, but with deterministic ordering. The DSL is very good for being human-readable as well as system-readable, and once you sort out the ordering problem then a standard diff tool is remarkably effective.

maximveksler commented 3 years ago

Thanks for the discussion guys, indeed worth consideration.

@simonbrowndotje I believe it could be an interesting approach (as well as useful dog food) to open source the design of structurizr.com using well.. structurizr.com. Would be interested in your thoughts on this? This might allow further discussion on issues such as the above (i.e. lock and merge strategies).

Regarding this PR, what fixes are required, or should this be merged?

simonbrowndotje commented 3 years ago

Diagrams for the cloud service and on-premises installation are linked to from the Structurizr help page. What's the purpose of the PR though ... are you building some tooling on top of the Python library? If not, you don't need to call lock and unlock from your Python program if you're using a free workspace.

Midnighter commented 3 years ago

Regarding this PR, what fixes are required, or should this be merged?

As mentioned I would like to combine this with #62. I can do so later today, unless you're very keen on taking this on?

What's the purpose of the PR though ... are you building some tooling on top of the Python library? If not, you don't need to call lock and unlock from your Python program if you're using a free workspace.

One problem that I need to correct is that the current example for uploading workspaces uses the context and thus locking.

yt-ms commented 3 years ago

In terms of the overall approach, it feels a bit odd for the lock and unlock methods to be returning a paid_plan flag. Can this be ascertained when the workspace is initially fetched so it is then just a property on StructurizrClient rather than having to return it from lock/unlock?

yt-ms commented 3 years ago

I've now incorporated handling lock errors on free plans into PR https://github.com/Midnighter/structurizr-python/pull/74 which also does the syntax changes for issue https://github.com/Midnighter/structurizr-python/issues/62.

Midnighter commented 3 years ago

Thank you for your contribution @maximveksler. I'm closing this one.