search

energinet-digitalisering / ksail

A CLI tool for provisioning GitOps enabled K8s clusters in Docker.
MIT License
0 stars 0 forks source link

CLIRunner fails to detect return code correctly and always returns 0 as exit code #3

Open AlmiS opened 5 months ago

AlmiS commented 5 months ago

Description of the bug

It seems the CLI runner reports an exit code 0, even when the command returns something else:

https://github.com/energinet-digitalisering/ksail/blob/main/src/KSail/CLIWrappers/CLIRunner.cs Line 34.

devantler commented 5 months ago

Thanks for isolating the issue! I was aware of the behaviour "it will always appear to successfully decrypt/encrypt if pointed at a file", but hadn't had time to find out why it happened.

devantler commented 4 months ago

@energinet-digitalisering/substation-data anyone willing to test if this works as expected on v1.4.20? 🙏

rvangsgaard commented 4 months ago

Our team has a DevOps session tomorrow where this issue will come up. I will update this issue afterwards.

rvangsgaard commented 4 months ago

Ksail now fails when SOPS command fails, which I believe is expected behaviour.

ksail sops huginn-corp-dev --encrypt k8s/clusters/huginn-corp-dev/variables/variables-sensitive.sops.yaml
🔐 Encrypting 'k8s/clusters/huginn-corp-dev/variables/variables-sensitive.sops.yaml'
✕ SOPS encryption failed

Consider propagating the error from the child command.

devantler commented 4 months ago

I wanted to propagate the command to stdout, but the SOPS CLI did not behave like regular CLIs, and it requires a workaround, which I'm not a big fan of. Also, I'm not really sure how to do it with the CLIWrap library I am using to call CLIs from code. The issue was that SOPS CLI did not populate its stderr and stdout. More specifically I do not expect the command to cause an exception, but instead fail gracefully. SOPS causes an exception in CLIWrap, wherein the stderr output is unavailable.

I was in my testing only able to get the output from .NET propagated to stdout, but that really wasn't very informative, and it also gave away stacktrace info, which seemed irrelevant to the user in this context.

devantler commented 4 months ago

What do you think? Would it be valuable to you if I gave it another try?

rvangsgaard commented 4 months ago

Generally good error reporting can save the day.

Fortunately I have direct access to the author of this library - If I did not, I would argue for good error reporting :)