Add support for lockable resources#940
Conversation
cdleonard
force-pushed
the
lockable-resources
branch
4 times, most recently
from
a9c0059 to
4a16129
Compare
|
This went through several force-pushes as I added more features and fix linting issues. Current version is stable |
| res = opener.open(request) | ||
| Requester.AUTH_COOKIE = res.cookie | ||
|
|
||
| _lockable_resources: Optional[LockableResources] = None |
There was a problem hiding this comment.
Can you help me understand usage here?
Instead of just using a normal property method
There was a problem hiding this comment.
It is meant to prevent creating multiple instance of the LockableResources class for each Jenkins instance - because why would you want that? But looking at how other entities are handled it is seems that only for Jobs a single instance is held inside Jenkins.jobs_container.
For consistency it makes sense to switch to a get_lockable_resources() method.
There was a problem hiding this comment.
Switched to a get_lockable_resources() method
| return f"Lockable Resources @ {self.baseurl}" | ||
|
|
||
| def get_jenkins_obj(self) -> "Jenkins": | ||
| return self.jenkins |
There was a problem hiding this comment.
Why not use just obj.jenkins?
There was a problem hiding this comment.
Most other JenkinsBase subclasses do this. The base JenkinsBase.get_jenkins_obj raises NotImplementedError so implementing this seems to be required.
Make JenkinsBase should just have a jenkins: Jenkins member? But that would be an unrelated refactor.
|
|
||
| @property | ||
| def _requester(self) -> Requester: | ||
| return self.jenkins.requester |
There was a problem hiding this comment.
Don't think we need this
|
|
||
| def poll(self, tree=None) -> None: | ||
| super().poll(tree) | ||
| self._data_dict = None |
There was a problem hiding this comment.
Why set this to none?
There was a problem hiding this comment.
It's cache invalidation.
The super().poll() call will update self._data and self._data_dict is generated based on self._data upon request (inside the data_dict getter method).
cdleonard
force-pushed
the
lockable-resources
branch
from
4a16129 to
55ffa69
Compare
|
Rebased addressing review comments and integrating a some other changes:
The RetryConfig class is very basic, the purpose of this abstraction is to allow more complex implementations - for example based on tenacity. I wanted to avoid adding dependencies. |
… various ResourceSelectors
cdleonard
force-pushed
the
lockable-resources
branch
from
34ca619 to
0a64ca7
Compare
|
rebased to fix codacy |
|
@clintonsteiner I see you merged trunk into this branch, but codacy now fails. Should I go fix it? It's been a while since I posted the PR but I'm still interested in getting this merged, and also #939. I believe I already addressed all of your comments. |
|
Merged this in due to a conflict on another branch, but left your commits, and name intact on the work |
This adds a new class for managing lockable resources from python. It includes some helpful context managers so you can do stuff like this:
The intended use case is hardware reservation systems backed by jenkins. There is no CLI.
I also found a similar implementation as external package: https://gitlab.com/alexandre-perrin1/jenkins-lockable-resources. It seems unmaintained, I was unable to install due to dependency conflicts and such.
This PR contains full systest support so the CI will test the code against a running jenkins instance. Hopefully this makes it easy to maintain.