[DPE-6296] Pyright fixes + structured_config additions + break down of actions.py
#7
Conversation
Adds the build & release CI
There was a problem hiding this comment.
The PR title is a bit misleading at this point 😂 . I was only expecting some libs and the gitignore updates, but here there are a bunch of things, as listed in the PR description that I would have honestly splitted in separate PRs on their own, since they seem rather orthogonal, and produce a lot of changes hard to review all in one go IMHO.
I don't think I have any concern with any of the changes in the PR itself, more the process and the best-practice to try to keep PR well-perimetered.
The only major concern or todo is that if we want the pyright checks to be part of this PR, we should also add the CI for it. Actually I have noticed that we are also missing CI in this repo, despite we already have some unittests.
So all-in-all, my suggestions here would be to:
- Rescope this PR to only add missing code, update gitignore
- Raise a PR to add CI with basic things: like linting and unittests (for which we should already have things)
- Raise a PR to add pyright compliance, and adding this check to the CI
- Add separate PRs for StructuredConfig and general refactoring (e.g. ActionHandlers)
By doing this, we would probably also make sure we have a process and we don't have regressions when we do the refactorings in point 4.
phvalguima
force-pushed
the
DPE-6101-add-libs-and-gitignore
branch
from
83a4ff9 to
369d824
Compare
phvalguima
force-pushed
the
DPE-6101-add-libs-and-gitignore
branch
from
369d824 to
52fd6a3
Compare
phvalguima
changed the title
structured_config
phvalguima
changed the title
structured_configstructured_config additions + break down of actions.py
| return None | ||
|
|
||
|
|
||
| class _LifecycleStateFactory: |
There was a problem hiding this comment.
suggestion what is the point of having an object here? We don't even have an init method or any attribute that we would use in the build method. I would just create a function build without the need of having an object (which as far as I can see, it does not provide much value) or possibly being a method of LifeCycleManager
There was a problem hiding this comment.
Fair point, I will move it to a function only and move it to a "match" statement.
|
|
||
| def next( | ||
| self, transition: DPBenchmarkLifecycleTransition | None = None | ||
| ) -> Optional["_LifecycleState"]: |
There was a problem hiding this comment.
praise I personally like that you splitted the next logic into separate objects with better defined logic. I believe it makes things easier to reason about and even debug
I just have a question...what does it mean to transition and return a None state? I'd probably have a State also for the null case (could one use the UNSET state that we already have here?)
If we have exception happening here, I would rather raise expection over just passing None, which is not very expressive
There was a problem hiding this comment.
Yes, the first sequence of ifs and elses rapidly became impossible to reason!
I want to keep the idea of a graph, or more specifically, a petri net: vertices represent the different states whereas the edges represent the transitions of the system.
For now I am just mapping "actions" to "transitions". Anything else (e.g. an error in a service) is a transition as well but I am not explicitly mapping right now. So, the "None" is just a generic bag for these other changes along the system.
If we receive None, then each lifecycle class does a series of pre-checks on the system: checks workloads, relation status, etc etc
| return None | ||
|
|
||
|
|
||
| class _FailedLifecycleState(_LifecycleState): |
There was a problem hiding this comment.
niptick, question why do we want these to look like they are private? Is this something the rest of the charm should not care about? given in python there are really no private objects/methods, I would possibly avoid this.
There was a problem hiding this comment.
When I wrote these set of classes I had the concept of "friend", borrowed from C++ in my mind...
The idea are a group of classes that are really closely tight together. To avoid a mess, I decided to make it explicit these classes here are unaccessible by anyone else (hence the "_" format).
| password=None, | ||
| security_protocol="SASL_PLAINTEXT", | ||
| replication_factor=1, | ||
| ) |
There was a problem hiding this comment.
suggestion This client would never work, wouldn't it? I would rather return None if the client can't be instantiated, such that people can just do:
if client:
# do something
Otherwise they may actually try to use something that we already know that it does not work
There was a problem hiding this comment.
Yes, that is fixed on the PR on TLS support, as I am doing all my tests with that final one...
This PR brings multiple pyright fixes and adds structured_config as a model to deconflict type checks.