Give the default failure "reason" when commands failed

[rina23q]

Proposed changes

Without giving the default reason, mappers will parse an error when command status is failed but no "reason" is given. However, "reason" filed should be optional in command payloads.

This bug was found during writing a unit test for firmware_update support.

[te/device/main///cmd/firmware_update/c8y-mapper-2023-11-16T14:02:31.419901262Z] {
   "status":"failed",
   "remoteUrl":"https://speed.hetzner.de/10GB.bin",
   "name":"10GB",
   "version":"2.0.0"
}
[te/errors] missing field `reason` at line 6 column 1

The bahaviour is covered by the unit test handle_log_upload_executing_and_failed_cmd_for_main_device().

Maybe worth adding a robot test?

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
• Documentation Update (if none of the other choices apply)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

• I have read the CONTRIBUTING doc
• I have signed the CLA (in all commits with git commit -s)
• I ran cargo fmt as mentioned in CODING_GUIDELINES
• I used cargo clippy as mentioned in CODING_GUIDELINES
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Further comments

[codecov]

Codecov Report

Merging #2463 (b53f427) into main (6fe2820) will increase coverage by 0.0%.
Report is 17 commits behind head on main.
The diff coverage is 100.0%.

Additional details and impacted files

Files	Coverage Δ
crates/core/tedge_api/src/messages.rs	93.5% <100.0%> (+<0.1%)	⬆️
crates/extensions/c8y_mapper_ext/src/log_upload.rs	87.6% <100.0%> (-0.1%)	⬇️

... and 61 files with indirect coverage changes

[didier-wenzek]

I'm not happy with this message which raises the question given by who.

That said I agree, it's hard to come with something sensible. "Unknown error"? In any case, as a user, this is frustrating.

[rina23q]

User will see this message only when a command is marked with "failed" but no "reason" is specified. If any internal error occurs inside thin-edge product, we always give the specific error message. I'm fine to change the message to anything, but I believe setting the default reason is correct. Otherwise, user might get "missing field reason at line 6 column 1" error, which is really not okay.

[didier-wenzek]

A default is indeed required.

[albinsuresh]

Making the reason was indirectly solving that problem, right? It forces the users to provide a reason when there's a failure (as long as it is mandatory only for the failed state). When there's a failure, the user has to create a JSON message with the status payload anyway. So, why not make him add the reason field as well? Why give an option to the user to shoot himself in the foot?

[didier-wenzek]

So, why not make him add the reason field as well? Why give an option to the user to shoot himself in the foot?

The issue highlighted by @rina23q is that if by mistake no reason is given then the final error message is more then puzzling ( "missing field reason at line 6 column 1"). A default "Unknown reason" string will be clearer both for the agent developer and the end-user.

Suggested change

	"No failure reason is given".to_string()
	"Unknown reason".to_string()

[rina23q]

So, why not make him add the reason field as well?

I believe that reason should not be mandatory. Highly recommended to add the reason for the failure though.

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
362	0	3	362	100	59m40.632999999s

[didier-wenzek]

Approved.