Add new API for loading config by content set label #44
Conversation
negillett
requested review from
danrodrig,
JayZ12138 and
rbikar
as code owners
Previously, a file name was needed in order to load a specific configuration file. Now one may provide a content set label instead.
negillett
force-pushed
the
7374
branch
2 times, most recently
from
956058e
to
e42f84d
Compare
negillett
requested review from
rohanpm,
alexandrevicenzi,
chandwanitulsi,
litleleprikon,
midnightercz and
rajulkumar
Removed storing of raised errors and replaced their subsequent calls with a new ConfigNotFound exception.
There was a problem hiding this comment.
Assuming that load_from_cs_label is intended as new public API:
- It ought to have a doc string
- The same API should be available for any kind of loader - right now you've fragmented the API such that this new method is only available for the gitlab loader. So, for example, you can't use it on a local copy of config files.
- This has added a new silent requirement that files are named with a certain scheme (filename matching up with content set name), yet this isn't documented anywhere and nor is it enforced by any check on the repo. It's also a redundant requirement as all the config files contain the content set labels within themselves. So this is now forcing two pieces of data to be maintained in sync with each other.
Regarding that last point - as I commented on the related JIRA issue, I really think we should not do that unless there is some strong reason to (e.g. is it going to solve a performance issue which otherwise has no reasonable solution).
The way I'd actually suggest implementing it is to:
- implement this on the shared Loader class
- make it work by calling load_all(recursive=True) and filtering the result
In other words, there is an implementation on the base class which loads all the config, then filters to just the requested content set(s).
Specific Loader implementations could potentially override it for a faster implementation, e.g. if loading each config file is expected to be slow. Ideally, that would be done without introducing some new requirement on the naming of files.
Where it previously attempted to compute the file name for loading, the new API will now load all config files and return only the one in which the label was found, as file names may be inconsistent.
Aren't these items for a release? |
Nope. |
Okay. I don't think we're proceeding with this issue #42 but I'll keep that in mind. |
| @@ -9,6 +9,12 @@ def test_no_load(): | |||
| Loader().load('something.yaml') | |||
|
|
|||
|
|
|||
| def test_no_load_by_cs_label(): | |||
There was a problem hiding this comment.
This test is passing because we are calling load_by_cs_label from Loader in base class, which calls load_all, from base class too, and that's the method that is raising the NotImplementedError. So the test is asserting that we cannot call load_by_cs_label from base class. Can you update the doc string?
| @@ -51,6 +50,10 @@ def load_all(self, recursive=False): | |||
|
|
|||
| return ubi_configs | |||
|
|
|||
| def load_by_cs_label(self, cs_label): | |||
There was a problem hiding this comment.
This can be removed, as we are using the implementation from Loader.
| @@ -44,6 +44,10 @@ def load_all(self, recursive=False): | |||
|
|
|||
| return ubi_configs | |||
|
|
|||
| def load_by_cs_label(self, cs_label): | |||
There was a problem hiding this comment.
Same as the one for GitLab, can be removed.
| def load_by_cs_label(self, cs_label): | ||
| """ | ||
| Load all the config files from repo and return one containing config for | ||
| the provided content set label. |
There was a problem hiding this comment.
I don't think we should expose the implementation of the method here. Rather, would be better to comment only on the outcome. It returns an ubiconfig matching the input or output content set label. Raises if there is no config for that label.
Previously, a file name was needed in order to load a specific
configuration file. Now one may provide a content set label instead.
Fixes #42