Men 8382 mender artifact fails on calling mender update show provides #703
Conversation
…sudo and properly handle sudo asking for password. Changelog: Title Ticket: MEN-8382 Signed-off-by: Michal Kopczan <michal.kopczan@northern.tech>
…ove it to cli/utils/ssh.go Changelog: None Ticket: MEN-8382 Signed-off-by: Michal Kopczan <michal.kopczan@northern.tech>
|
@michalkopczan, start a full client pipeline with:
my commands and optionsYou can prevent me from automatically starting CI pipelines:
You can trigger a client pipeline on multiple prs with:
You can trigger GitHub->GitLab branch sync with:
You can cherry pick to a given branch or branches with:
|
cli/write.go
Outdated
| return nil, nil | ||
|
|
||
| // Wait for 60 seconds for ssh to establish connection | ||
| err = waitForBufferSignal(stdout, os.Stdout, sshInitMagic, 2*time.Minute) |
There was a problem hiding this comment.
Looks like 2 minutes and thus 120 seconds to me.
There was a problem hiding this comment.
Yeah, I think the comment should say 120 seconds instead
There was a problem hiding this comment.
Again, I think the code looks good :)
This is perhaps a personal preference, feel free to dismiss, but personally I would prefer a bit shorter commit titles and rather do something like:
fix: call show-provides with sudo
Changelog: Fix crash when generating artifact from ssh by adding sudo to show-provides and properly handle sudo asking for password
cli/write.go
Outdated
| if err == nil { | ||
| sigChan = make(chan os.Signal, 1) | ||
| errChan = make(chan error, 1) | ||
| // Make sure that echo is enabled if the process gets | ||
| // interrupted | ||
| signal.Notify(sigChan) | ||
| go util.EchoSigHandler(ctx, sigChan, errChan, term) | ||
| } else if err != syscall.ENOTTY { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Are we intentionally ignoring all errors except ENOTTY?
There was a problem hiding this comment.
Huh, it also caught me by surprise - that's exactly what I though when I first saw this code (it was there before, I just moved it to new file). But this is not the case. Actually, we're ignoring ENOTTY exclusively.
- if there's no error, we set up echoSigHandler to re enable echo on signal
- if there's any error except ENOTTY, we return an error
- if there's ENOTTY error, we ignore it, because there's no need to disable echo if there's no TTY
|
|
||
| type SSHCommand struct { | ||
| Cmd *exec.Cmd | ||
| ctx context.Context |
There was a problem hiding this comment.
The ctx is unused.
| ctx context.Context |
Side-note; storing Context in structs is in most cases considered an anti-pattern in Go
There was a problem hiding this comment.
Thanks for the tip about anti-pattern.
I thought that by adding ctx here even though it's unused, I am making sure that the context lives long enough to be used in EchoSigHandler. Now I understand this is wrong.
Can you confirm that supplying _ctx to EchoSigHandler makes sure that the lifetime is extended so that EchoSigHandler can handle it every time, even if the app is being closed?
| case <-time.After(deadline): | ||
| err = errors.New("Input deadline exceeded") |
There was a problem hiding this comment.
We could also reuse the deadline from the context in the parent scope here instead of a fixed duration.
| case <-time.After(deadline): | |
| err = errors.New("Input deadline exceeded") | |
| case <-ctx.Done(): | |
| err = ctx.Err() |
There was a problem hiding this comment.
I'm not sure I understand, sorry - new to Go and new to the code above (just moved, not written myself)
Do you mean to reuse context as in, "if the execution finished, context was marked as Done, we return an error in waitForBufferSignal"?
My understanding here is that we wait either for error, or for 120 seconds for user to provide a root password over ssh. If user does not provide a password within 120 seconds, we return an error. I don't understand how reusing context could be used here.
Changelog: Fix crash when generating artifact from ssh by adding sudo to show-provides and properly handle sudo asking for password Ticket: MEN-8382 Signed-off-by: Michal Kopczan <michal.kopczan@northern.tech>
|
Merging these commits will result in the following changelog entries: Changelogsmender-artifact (MEN-8382_new-mender-artifact_fails_on_calling_mender-update_show-provides)New changes in mender-artifact since master: Bug Fixes |
|
@michalkopczan, start a full client pipeline with:
my commands and optionsYou can prevent me from automatically starting CI pipelines:
You can trigger a client pipeline on multiple prs with:
You can trigger GitHub->GitLab branch sync with:
You can cherry pick to a given branch or branches with:
|
|
Added new commit, with format suggested by Daniel. I'll rewrite the commits after the review is finished to comply with our guidelines, i.e. two commits - one for fix, one for refactoring. |
New PR, disregard the old one (701)
Added two separate commits, one for the fix, another for ssh refactoring.
Compared to previous version, I removed fixes for restoring echo and hanging ssh in case remote device fails silently (e.g. power loss). Will move it to new tasks.