Support free tier #73
Conversation
| Hi @maximveksler, Thank you for the contribution. Maybe there are multiple approaches to take here:
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. |
| @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. 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? |
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 The diff display plus confirmation is not a lot of work, though, so that could be implemented.
Indeed the response from the API is always |
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. |
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.
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. |
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? |
| 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. |
| 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 Regarding this PR, what fixes are required, or should this be merged? |
| 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 |
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?
One problem that I need to correct is that the current example for uploading workspaces uses the context and thus locking. |
| | ||
| return self._paid_plan(response), response.success | ||
| | ||
| def unlock_workspace(self) -> bool: |
There was a problem hiding this comment.
Return type needs updating to reflect the new values returned.
| ) | ||
| return response.success | ||
| | ||
| return self._paid_plan(response), response.success |
There was a problem hiding this comment.
Return type of the method needs updating to match
There was a problem hiding this comment.
I'd also be tempted to put the success first as it's the more significant of the two values.
| 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? |
| Thank you for your contribution @maximveksler. I'm closing this one. |
4 participants
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.