Single-point for reading container configuration #242
Conversation
This is a plain code move from `containers_list` into a standalone function. Added value is - reusability - documentation, incl. type annotation Also adds rendering of the utility module's documentation. This is a first step towards resolving datalad#238
This is a common use case in commands other than `containers_list`. Albeit not strictly necessary (all configuration could be read and processed first, and sub-selection could happen in user code), it is a low-complexity change. Moreover, we envision more configuration post-processing to happen in the future (see datalad#238). This would change the cost assessment of loading everything upfront.
This changeset maintains the exact behavior of the command, and merely adds documentation and replaces custom configuration access with the new `get_container_configuration()` helper. The documentation may communicate suboptimal command semantics, but these were given before, and are only written down now. Also see datalad#240 for additional aspects re a future refactoring. Configuration writes now use the ``scope=branch`` approach. It was introduced with datalad v0.16 and requires no dependency adjustments.
...for the purpose of a standardization of container configuration reads on `get_container_configuration()`.
This is done by turning of the result rendering. This revealed that the command implementations cause uncondtional result rendering datalad#241
This is the last change for establishing `get_container_configuration()` as the single code piece for reading container documentation. This opens the door for implementing on-read normalization of configuration items, such as the image path. Closes datalad#238
|
Code Climate has analyzed commit 8c6dba5 and detected 0 issues on this pull request. View more on Code Climate. |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #242 +/- ##
==========================================
+ Coverage 94.69% 94.75% +0.05%
==========================================
Files 25 25
Lines 1093 1105 +12
==========================================
+ Hits 1035 1047 +12
Misses 58 58
☔ View full report in Codecov by Sentry. |
Possible a little less complex to understand.
| @@ -329,6 +330,7 @@ def __call__(name, url=None, dataset=None, call_fmt=None, image=None, | |||
| return | |||
|
|
|||
| # store configs | |||
| cfgbasevar = "datalad.containers.{}".format(name) | |||
There was a problem hiding this comment.
| cfgbasevar = "datalad.containers.{}".format(name) | |
| cfgbasevar = f"datalad.containers.{name}" |
| url = container_cfg.get("updateurl") | ||
| if not url: |
There was a problem hiding this comment.
not necessary for this PR but I realized that we can enter the XXI century now that 3.8 is the minimal supported version (forced at datalad core level but not yet in this extension setup.cfg), and can start using walrus assignment operator, so smth like
| url = container_cfg.get("updateurl") | |
| if not url: | |
| if not (url := container_cfg.get("updateurl")): |
| name: str, optional | ||
| If given, the reported configuration will be limited to the container | ||
| with this exact name. In this case, only a single ``dict`` is returned, | ||
| not nested dictionaries. |
There was a problem hiding this comment.
could we make it more explicit, i.e. separate out a get_containers_configuration(ds) ? internally get_container_configuration(ds, name) can just call that get_containers_configuration(ds), I don't care. But having singular semantic in the name but getting a dict for multiple containers will eventually confuse someone like me at some point because e.g. I thought without a name specified it would return the config for the "only" container (as e.g. containers-run behaves IIRC) and raise KeyError or smth like that if multiple are found -- so it becomes ambiguous.
There was a problem hiding this comment.
Just to clarify. Your response to the description in dbb2f91 (commit message) is: Let's nevertheless always read and post-process all configuration, make all filesystem checks for all containers, and then throw away all that information, but for one container.
Rather than that, you can also get that semantic cleanliness by having two near-copies of that function.
Just to be clear, I am not here to argue -- I just need this merged, and I don't care in which way. I just do not want to guess what should be done and avoid future iterations on this code.
There was a problem hiding this comment.
I have missed that there is a continue shortcut, so indeed some code is skipped... even though complexity stays O(N), there would indeed be not only a waste of cycles but also possibly errors on unrelated containers which should be avoidable. I guess a way could be to make current function _private and then expose it via two clean interfaces calling it out with or without a name.
I will leave it up to you whether to implement that or proceed with current version.
There was a problem hiding this comment.
This is a nice suggestion! I will change it to use a private helper and two clean interfaces.
Depending on your reviewing capabilities, I might do this in a separate follow-up PR, given that more PR are built on this, and they would all need an update.
This changeset established a new helper
get_container_configuration()as the single method to read configuration on containers.This closes #238
This changeset is the precondition for implementing configuration item normalization effciciently -- for example as needed for resolving #224
A number of issues have been detected while working on this changeset, which are not addressed in here, but will be the topic of future work:
result_renderer='disabled'for internal datalad command execution #241datalad-removeincontainers-remove#240register_config()#239This changeset implies no behavior changes. The only user-facing change is an expanded documentation.
9b95b78 is not strictly needed in this PR. It led to #241 and could be included in a future PR. However, I see no benefit it such a diff to accumulate dust.