-
Notifications
You must be signed in to change notification settings - Fork 239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for is_offline? bug reporting all nodes as being offline #165
Conversation
Sorry for the delay in getting to this PR. I believe I used to get a string when I initially wrote that code. It is great if we get a boolean but I just wanted to make sure we shouldn't break any backwards compatibility with this change. We could either confirm if this behavior was not changed recently by Jenkins or check for both types. What do you think? |
Hey, thanks for getting back to me. No problem regarding the wait - I ended up using the Jenkins API directly for my very simple use case. Yeah I find it hard to believe that you overlooked this when you wrote this gem originally so I'm sure the API has changed in the interim. I was testing against a recent Jenkins version ( Like you mentioned, a more practical approach for now might be to change my PR to accept a string or boolean. I think moving forward it might be a good idea to have more explicit templated API responses for use in your unit tests - obviously these require some upkeep to prevent API drift which might be costly in terms of time. Ultimately I suppose it depends how invested you want to be as the Jenkins API changes. Ben |
…th bool and string case
Restoring node property methods to its original logic, but handles both bool and string case
Hi @arangamani, can we consider merging @luchenghan's changes? Both strings and booleans are now supported like we discussed above (sorry I forgot to come back and finish this). |
@bsnape Sure! |
Fix for is_offline? bug reporting all nodes as being offline
I'll release a new version in a day or two. Thanks all! |
Wonderful! Thanks @bsnape and @arangamani ! |
Hi,
This is a fix related to #164.
Please let me know what you think and if you'd like any changes to be made.
Ben