Fixes and improvements #864
Conversation
|
I reuse someone else's comment that lead to updates: Some feedback so we can integrate, or discuss, on this ? |
jenkinsapi/jenkins.py
Outdated
| not hasattr(self, "_get_plugins") | ||
| or self._get_plugins.baseurl != url | ||
| ): | ||
| self._get_plugins = Plugins(url, self) |
There was a problem hiding this comment.
I'd prefer something like self._plugin_cache to make it clear that it is data and not a method.
jenkinsapi/plugins.py
Outdated
| update_center = ( | ||
| "https://updates.jenkins.io/update-center.actual.json" | ||
| ) | ||
| jsonp = requests.get(update_center).content.decode("utf-8") |
There was a problem hiding this comment.
Why not directly use the json() method on the response?
There was a problem hiding this comment.
When I wrote this the first time, I didn't know about this possibility.
| @@ -37,7 +38,7 @@ def __eq__(self, other) -> bool: | |||
| return self.__dict__ == other.__dict__ | |||
|
|
|||
| def __str__(self) -> str: | |||
| return self.shortName | |||
| return f"{self.shortName}@{self.version}" | |||
There was a problem hiding this comment.
This change is causing a test failure
There was a problem hiding this comment.
=========================== short test summary info ============================
FAILED jenkinsapi_tests/unittests/test_plugins.py::TestPlugins::test_plugin_repr - AssertionError: '<jenkinsapi.plugin.Plugin subversion@Unknown>' != '<jenkinsapi.plugin.Plugin subversion>'
…gin should be downloaded from
for more information, see https://pre-commit.ci
| @@ -37,7 +38,7 @@ def __eq__(self, other) -> bool: | |||
| return self.__dict__ == other.__dict__ | |||
|
|
|||
| def __str__(self) -> str: | |||
| return self.shortName | |||
| return f"{self.shortName}@{self.version}" | |||
jenkinsapi/plugins.py
Outdated
| def update_center_dict(self): | ||
| update_center = "https://updates.jenkins.io/update-center.json" | ||
| jsonp = requests.get(update_center).content.decode("utf-8") | ||
| return json.loads(jsonp_to_json(jsonp)) | ||
| if not hasattr(self, "_update_center_dict"): | ||
| update_center = ( | ||
| "https://updates.jenkins.io/update-center.actual.json" | ||
| ) | ||
| self._update_center_dict = requests.get(update_center).json() | ||
| return self._update_center_dict | ||
|
|
||
| @property | ||
| def update_center_dict_server(self): | ||
| if not hasattr(self, "_update_center_dict_server"): | ||
| self._update_center_dict_server = self.jenkins_obj.requester.get_url( | ||
| self.jenkins_obj.get_update_center_url(2) | ||
| ).json() | ||
| return self._update_center_dict_server |
There was a problem hiding this comment.
I am concerned about this part, and anywhere I added caching. as it may have impact on code that uses Plugins.update_center_dict and expect it to make a call.
There was a problem hiding this comment.
When in doubt, lets add more tests
I am trying hard to steer away from breaking changes
I'll take another look when I have time
There was a problem hiding this comment.
I'll have some time to look into tests. I had to add another method in plugins.py meanwhile (to avoid infinity loops when a plugin fails to update).
I'll update the PR with relevant tests for this too maybe next week.
Hi,
Please find here some improvements added to this, very good, library.
In order to use it efficiently and correctly, I had to integrate these updates.
I cherry-picked (to give credit to @mrrsm) the commit added in this PR.
I hope this can be merged, so I can switch to the real release, instead of my own repo.