search

jenkinsci / jenkins-charm

Juju charm to deploy and scale Jenkins
Other
17 stars 36 forks source link

hook fails on all Jenkins commands except quietDown and cancelQuietDown #91

Open axinojolais opened 4 years ago

axinojolais commented 4 years ago

Hello !

Perhaps I'm missing something obvious, because it's strange that no one saw that before ?

In the following block, the statement in the if cannot be true unless action is either quietDown or cancelQuietDown. So return will not be called, and the else statement from the try block will be executed, leading to a hook error.

https://github.com/jenkinsci/jenkins-charm/blob/734965f2e2f819e79e0fa8693bc77d4e787371b1/lib/charms/layer/jenkins/api.py#L214-L222

I'm not sure what the intent is here, will investigate and make a PR

axinojolais commented 4 years ago

According to https://python-jenkins.readthedocs.io/en/latest/api.html, jenkins_open returns a str so I have no idea how the original version of _execute_action ever worked : https://github.com/jenkinsci/jenkins-charm/blob/155d2414662deb5f5643a677e479c1f2b18634d1/lib/charms/layer/jenkins/api.py#L159-L167

alejdg commented 4 years ago

All actions, except for those two, when executed return HTTPError. This is a "normal" behavior in Jenkins.

So when things happen as expected all actions will fall into the except part. Only when something different occurs the else will part will be executed.

axinojolais commented 4 years ago

Well the action safeRestart on the Jenkins I'm looking at (version 2.235.1) doesn't return HTTPError so the raise RuntimeError is reached. Every single time.

alejdg commented 4 years ago

@axinojolais maybe jenkins has changed the return for that action, possibly for all of them.

So this issue is definitely valid and we should check what implemented actions are failing due to that and add them to the same place as quietDown and cancelQuietDown: https://github.com/jenkinsci/jenkins-charm/blob/734965f2e2f819e79e0fa8693bc77d4e787371b1/lib/charms/layer/jenkins/api.py#L217

axinojolais commented 4 years ago

@mthaddon @darkalia I don't agree this is fixed. #93 only fixes safeRestart. What about the other commands ? Have we tested them ? Do they work ?

ben-ballot commented 4 years ago

@mthaddon @darkalia I don't agree this is fixed. #93 only fixes safeRestart. What about the other commands ? Have we tested them ? Do they work ?

I agree it's not fixed. It's only part of the fix to unstuck a situation. My bad for using "Fixes: #91" in the commit message. We should check for those commands as well: