feat: use docker commands for stack handling (DEV-1530) #261
Conversation
Co-authored-by: Balduin Landolt <33053745+BalduinLandolt@users.noreply.github.com>
There was a problem hiding this comment.
LGTM, I think this is a great improvement!
In general (doesn't need to be part of this PR):
- documentation: we should think about the structure and content (target) of the documentation
code organization: you added all scripts under--> as discussed, you can ignore that, I saw now that you actually deleted all the previous shell scriptsknora.dsplib.utilsbut I would keep these things bundled and separated from the other code in their own package / module- naming: I would consistently call our components by their name, i.e.
DSP-APIinstead ofAPIandDSP-APPinstead ofAPP, especially when using capital letters, because that's their product name. If usingapiorapponly, I would use lower-case to reference the thing they are, which is an api / app like any other api / app.
Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>
- docker system prune after startup - improve documentation - update version of API
…for-stack-handling' into wip/dev-1530-use-docker-compose-for-stack-handling
DEV-1530 Update make command in docker-json-server repo
update build command to use buildx so that we can create images for different architectures |
Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>
Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>
knora/dsplib/utils/stack_handling.py
Outdated
| raise BaseError("max_file_size must be between 1 and 100000") | ||
| max_post_size_regex = r"max_post_size ?= ?[\'\"]\d+M[\'\"]" | ||
| if not re.search(max_post_size_regex, docker_config_lua_text): | ||
| raise BaseError("sipi.docker-config.lua doesn't contain a variable max_post_size that could be replaced") |
There was a problem hiding this comment.
Relating to our discussion regarding user feedback etc.: This is a good example where the current setup is not optimal.
In the case of the first Exception, the user has passed an invalid parameter (user's fault, should get a gentle explanation how to do it better), in the case of the second exception, something in a config file you loaded from the API repo was not as expected (absolutely not the user's fault, they can't do anything about it).
But both instances behave exactly the same: An exception is thrown and never caught - so the user will see a an intimidating stack trace.
So in terms of user feedback this is definitely not nice. And in terms of functionality, I'm not sure if not being able to set the max file size should stop the program from running in either of those cases.
There was a problem hiding this comment.
Thanks for the hints. One of my long-term ideas is to raise different kinds of exceptions in the code, and then catching the user-feedback-exceptions in dsp_tools.py, print the message, and then exit() without stack trace. Only the "real errors" would escalate uncaught and lead to a stack trace. Do you think that would be a good design pattern?
Or even easier: if an error occurs which is the user's fault, print message and exit(). Use BaseError only if an internal-caused error occurs.
I will do that in a separate PR, but what do you think about it?
There was a problem hiding this comment.
That's definitely a reasonable and common approach. You can then create an inheritance hierarchy of exceptions for different categories of errors, and things like that.
And it will definitely be a great improvement to how it is currently.
Personally I'm not the biggest fan of exceptions, but in Python it's not exactly easy to find better solutions. To me, exceptions always feel like "blowing up stuff", and there is nothing very "exceptional" about a user entering a wrong parameter... But as I said, there is no super-neat pythonic way of a controlled and typed error channel like we have in ZIO.
| docker_path = Path(__file__).parent / Path("../docker") | ||
|
|
||
|
|
||
| def start_stack( |
There was a problem hiding this comment.
a remark purely on code style: a function should never be 100 lines long (including docstring), it should never "do more than one thing" and it should normally not have to operate on different levels of abstraction. All three of those are the case for this function.
If you find yourself writing that kind of code (which is normal, that's how one writes code), then you should afterwards refactor it by extracting all sub-tasks and lower-level stuff into separate private functions.
(I'm freely quoting Bob Martin's "Clean Architecture" here - but it's a good rule of thumb to follow.)
There was a problem hiding this comment.
Thanks for the hint, it's important that I learn such stuff about architecture. I think I won't do it in this PR. But a general question concerning this rule of thumb: Doesn't it make the code more complicated if I have a lot of private methods that are so specific that are only called once?
There was a problem hiding this comment.
Bob Martin (and I) would say: no, your code should get more readable and easier to understand.
You have to get some things right though:
- a method name should be descriptive of what the method does
- a method must only operate on one abstraction level, namely calling methods of the next lower abstraction level
(and there are more finesses of course)
If you follow those rules, all your methods should turn out pretty much self explanatory: the name says what it does and the (maybe 3 to 10) function calls within it are self-explaining steps that need to be taken, to achieve this.
And if you are interested in further detail, you can jump into each of those steps, where the same thing will be true: the function name is what it does, and the contents are the steps taken to do it.
Does that make sense?
If you're interested, there is - among tons of other material - a several hour long talk series by "Uncle Bob" named "Clean Code" on Youtube, which is definitely worth watching.
jnussbaum
deleted the
wip/dev-1530-use-docker-compose-for-stack-handling
branch
resolves DEV-1530