Conversation
|
I reuse someone else's comment that lead to updates: Some feedback so we can integrate, or discuss, on this ? |
Achimh3011
left a comment
Achimh3011
left a comment
There was a problem hiding this comment.
Looks ok to me, two smaller comments.
jenkinsapi/jenkins.py
Outdated
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
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.
jenkinsapi/plugin.py
Outdated
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
jenkinsapi/plugin.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.
Achimh3011
left a comment
Achimh3011
left a comment
There was a problem hiding this comment.
Define all member variables in __init__ and use is not None as test. Otherwise fine.
|
Sorry missclick. |
3 participants
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.