NAS-131083 / 25.04 / service.stop return values are swapped

[creatorcary]

The API documentation states: 'Will return true if service successfully stopped' but the opposite is true.

Return True on successful stop, or raise a CallError if the service is still running.

Tests: http://jenkins.eng.ixsystems.net:8080/job/tests/job/api_tests/630/

[bugclerk]

Jira URL: https://ixsystems.atlassian.net/browse/NAS-131083

[anodos325]

Should we do this or fix the docs? The latter is less likely to cause a regression unless you're absolutely certain it won't break anything.

[bmeagherix]

If we're going to change the behavior then I'd suggest a CI run run as a prerequisite of the PR.

[creatorcary]

Should we do this or fix the docs? The latter is less likely to cause a regression unless you're absolutely certain it won't break anything.

Amazingly I only see one place in middleware where the return value is used and it's in a test, but I'm definitely not certain about anywhere else yet.

[anodos325]

Amazingly I only see one place in middleware where the return value is used

Our codebase is rarely that simple. It's a trap.

[anodos325]

If you go down the path of changing the return value, you should also first socialize change with the QE team as they may be explicitly testing this.

[creatorcary]

If you go down the path of changing the return value, you should also first socialize change with the QE team as they may be explicitly testing this.

Definitely, I'm just gauging opinion for now to see if this would be worth changing. To me a return value of True just intuitively means success

[themylogin]

I checked the UI code, they are using this method's return value too.

[mgrimesix]

My vote would be to update the docstrings to indicate what is returned.

[yocalebo]

I hate this (anti)-pattern. It has plagued us since freeBSD days 🙂. Why would we keep this behavior? Is there any logical reason (other than the apprehension to change)? I'm not going to die on this hill but I find it weird to keep something like this in code that is (and has) caused lots of confusion over the years.

[mgrimesix]

I'm not going to defend it. I find 44 usages of 'service.stop' in middleware. Maybe 1/3 to 1/2 in tests.

[jordan-mcgillivray005]

We assert status_code == 200 for service.stop. Changing the return to be True if successfully stopped (the correct behavior in my opinion) doesn't affect us.

[anodos325]

I hate this (anti)-pattern. It has plagued us since freeBSD days 🙂. Why would we keep this behavior? Is there any logical reason (other than the apprehension to change)? I'm not going to die on this hill but I find it weird to keep something like this in code that is (and has) caused lots of confusion over the years.

I'm not defending it, we just need to make sure that all stakeholders are aware (in addition to making sure we don't break our own CI :) )

[creatorcary]

Passing our CI tests: http://jenkins.eng.ixsystems.net:8080/job/tests/job/api_tests/630/

[yocalebo]

This looks good to me. Just make sure UI team is aware of the behavioral change (they might not care). You also need to make sure the TC team is aware of the change too.

[bugclerk]

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.