Add detailed error message in fluxctl sync #2765
Conversation
| @@ -58,6 +58,7 @@ type GitConfig struct { | |||
| Remote GitRemoteConfig `json:"remote"` | |||
| PublicSSHKey ssh.PublicKey `json:"publicSSHKey"` | |||
| Status git.GitRepoStatus `json:"status"` | |||
| Error string `json:"errors"` | |||
There was a problem hiding this comment.
This will break backwards compatibility with clients using the v6 endpoint.
You would need to bump the API version to extend it. But, even then, I am not sure that's the best place for the error (to be fair I don't think that the status should be part of the config either, bit that's how it is).
There was a problem hiding this comment.
Ah right, thank you for reviewing.
I can think of making git.GitRepoStatus a map containing the Error, or adding another endpoint for the repo status?
There was a problem hiding this comment.
After taking a look at it, adding the error is probably OK but you should add a new version of the data structure and a new version of the endpoint (which will, in turn, require bumping the latest version of the API)
There was a problem hiding this comment.
Since the Error field is optional, I don't think just adding it to the v6 API response breaks backward-compatibility. A way to reason about it is to do the matrix of old version / new version and see if mismatches will cause a problem.
In the following table, "Old" means "does not know about the Error field" and "New" means "expects to use the Error field".
| Old fluxd | New fluxd | |
|---|---|---|
| Old fluxctl | Operates as now | Will not deserialise Error field, and thereby operate as now |
| New fluxctl | Can act conditionally on presence of Error field | Operates as desired |
There was a problem hiding this comment.
OK, I think that's fine :) , @dinosk is (rightfully) going to hate me for it though.
There was a problem hiding this comment.
Sorry @dinosk! Blame me, I'm the trouble-maker.
So: to be clear, let's go back to how you initially had it, with an Error field added to v6.GitConfig, and that field being used if present in cmd/fluxctl/sync_cmd.go.
There was a problem hiding this comment.
😅 it is no problem! I see your point, I’ll revert it to the initial changes
There was a problem hiding this comment.
makes backup of new api version
Ok, this is back to the initial changes
There was a problem hiding this comment.
Thanks for being so understanding!
dinosk
force-pushed
the
issues/2657-fluxctl-sync-error-message
branch
10 times, most recently
from
2efb502 to
c61ec9f
Compare
dinosk
changed the title
dinosk
force-pushed
the
issues/2657-fluxctl-sync-error-message
branch
2 times, most recently
from
f5a7e36 to
1c904e9
Compare
|
Thanks! I would like @squaremo to also have a look In the meantime, can you fix the failing test? I thought it was a transient CI error, but it fails systematically |
dinosk
force-pushed
the
issues/2657-fluxctl-sync-error-message
branch
3 times, most recently
from
f628b49 to
f9e7fd3
Compare
|
Tests are fixed, let me know if I should add sth else! |
* Extends v6.GitConfig to include `Error` field. * In case of `fluxctl sync` error, prints the `Error` field in the output. Signed-off-by: Dinos Kousidis <dinkousidis@gmail.com>
dinosk
force-pushed
the
issues/2657-fluxctl-sync-error-message
branch
from
f9e7fd3 to
f42ab00
Compare
Extends v6.GitConfig to include
Errorfield.In case of
fluxctl syncerror, prints theErrorfieldin the output.
Closes #2657
Signed-off-by: Dinos Kousidis dinkousidis@gmail.com