fix: Detect insights-client status in non-legacy mode #125
Conversation
m-horky
force-pushed
the
cct-525/insights-client-status
branch
from
69fe879 to
98969eb
Compare
Shouldn't we rather fix |
|
I'm not sure if insights-client is broken if it returns 0. You are asking for a yes/no response, so it correctly returns "registered" or "non-registered" -- I'd expect other exit code if we couldn't talk to API or something similar. IMO, the legacy upload is bad -- we shouldn't be returning non-zero code there. |
|
The logic of returning zero, when system is connected, is correct. Returning non-zero code, when system is not connected for various reasons, is also correct. The |
|
There is really low risk of doing such change in I'm not willing to do wrong things in |
|
I'd like you to reconsider this PR. It seems too risky to change the return codes for insight-client. Since the behavior is delivered in a Core, it will be pushed everywhere, that's not how RHEL behaves. It wouldn't be responsible. We should include this patch, even though it's not as pretty as we'd have hoped. Unfortunately, the reality is what is is. |
|
I agree that poor code in insights-client should not create poor code in other projects. I also agree that exit codes from programs should be consistent. I also disagree with this patch, but more on the grounds that relying on output strings is dangerous. If a change in core later changes this output, suddenly rhc clients everywhere will start misbehaving. I am generally against tightly-coupling programs like this; it creates tenuous dependencies that become extremely complex to maintain. Making any change to insights-client comes with a very high risk of breaking other projects because of the exact reason I described in the previous sentence; it's terrible CLI has become a de-facto API that other tooling rely upon. Could we modify |
|
That's not a bad idea. |
|
It's not elegant, but the absence of those files can be more elegantly handled than string output changing without warning. |
m-horky
force-pushed
the
cct-525/insights-client-status
branch
2 times, most recently
from
ce5168d to
105e756
Compare
|
I want to wait for @jirihnidek opinion in this case before merging, since he raised concerns about the original implementation. |
m-horky
force-pushed
the
cct-525/insights-client-status
branch
2 times, most recently
from
9697f7c to
a3dcc54
Compare
insights.go
Outdated
| // asking Inventory, its two modes (legacy v. non-legacy API) behave | ||
| // differently (they return different texts with different exit codes). | ||
| // The `.registered` file is always present on the registered system. | ||
| _ = exec.Command("/usr/bin/insights-client", "--status").Run() |
There was a problem hiding this comment.
I would not ignore returned error here. E.g. If the file /usr/bin/insights-client did not exist, then I would return such error.
* Card ID: CCT-525 With `legacy_upload=True` mode, `insights-client --status` returns code 0 if the system is registered and 1 otherwise. In non-legacy mode, 0 is always returned. Even though the `/etc/insights-client/.registered` file is not a public API, it is reliable and we should be able to use it to detect the registration status.
m-horky
force-pushed
the
cct-525/insights-client-status
branch
from
a3dcc54 to
88c3457
Compare
| // When stderr is not empty, then we should return this as error | ||
| // to be able to print this error in rhc output | ||
| stdErr := errBuffer.String() | ||
| if len(stdErr) == 0 { |
There was a problem hiding this comment.
I would not delete old functionality altogether, because it will change behavior as you can see on this example:
root@localhost:/path/to/rhc# cat /usr/bin/insights-client
#!/bin/bash
exit 1
root@localhost:/path/to/rhc# ./rhc status
Connection status for thinkpad-p1:
● Connected to Red Hat Subscription Management
● Cannot detect Red Hat Insights status: exit status 1
● The yggdrasil service is active
Manage your connected systems: https://red.ht/connector
If insights-client returns non-zero value, can we return false, nil to be consistent with old behavior?
There was a problem hiding this comment.
I wouldn't say that would be correct, because you get non-zero code even on network errors. It might have been the previous behavior, but it could be misleading. I would rather report the cached value risking it's out of date then report it's not connected to insights because the network glitched up.
With
legacy_upload=Truemode,insights-client --statusreturns code0 if the system is registered and 1 otherwise. In non-legacy mode, 0 is
always returned.
Even though the
/etc/insights-client/.registeredfile is not a publicAPI, it is reliable and we should be able to use it to detect the
registration status.