Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stream output from KubeExec #1194

Closed
wants to merge 5 commits into from
Closed

Stream output from KubeExec #1194

wants to merge 5 commits into from

Conversation

PrasadG193
Copy link
Contributor

@PrasadG193 PrasadG193 commented Jan 18, 2022 •
edited

Change Overview

This PR contains changes for streaming stdout and stderr output from kube exec commands.
It contains refactoring done on top of #1037

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

  • #XXX

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E
$ go test -check.f="TestStderr" -check.vv .
START: exec_test.go:39: ExecSuite.SetUpSuite
PASS: exec_test.go:39: ExecSuite.SetUpSuite     7.783s

START: exec_test.go:80: ExecSuite.TestStderr
PASS: exec_test.go:80: ExecSuite.TestStderr     11.821s

START: exec_test.go:73: ExecSuite.TearDownSuite
PASS: exec_test.go:73: ExecSuite.TearDownSuite  0.277s

OK: 1 passed
PASS
ok      github.com/kanisterio/kanister/pkg/kube 19.914s

tdmanv and others added 4 commits January 17, 2022 10:25
@tdmanv @PrasadG193
Stream output from KubeExec
dca151e
@tdmanv @PrasadG193
Set error in pipe readers instead of a channel
bb0e747
@PrasadG193
Fix compile errors
35830e3 
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@PrasadG193
Handle returned error by exec stream
f409626 
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
PrasadG193
PrasadG193 commented Jan 18, 2022
View reviewed changes
pkg/kube/exec.go
Comment on lines +90 to +95
if oerr != nil {
err = errors.Wrapf(err, "Failed to read stdout: %s", oerr.Error())
}
if eerr != nil {
err = errors.Wrapf(err, "Failed to read stderr: %s", eerr.Error())
}
Copy link
Contributor Author

@PrasadG193 PrasadG193 Jan 18, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would make error with duplicate error statements. We can remove it or just log it in debug mode.

@pavannd1 pavannd1 requested a review from tdmanv January 19, 2022 01:42
pavannd1
pavannd1 reviewed Jan 20, 2022
View reviewed changes
pkg/kube/exec.go Outdated Show resolved Hide resolved
@PrasadG193
Don't add error while closing pipe
91a5567 
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@ihcsim ihcsim mentioned this pull request Mar 25, 2022
Merged
10 tasks
@shuguet shuguet added this to In Progress in Kanister via automation Mar 25, 2022
@shuguet shuguet moved this from In Progress to Review Required in Kanister Mar 25, 2022
@ihcsim
Copy link
Contributor

ihcsim commented May 5, 2022

Superseded by #1331.

@ihcsim ihcsim closed this May 5, 2022
Kanister automation moved this from Review Required to Done May 5, 2022
@ihcsim ihcsim deleted the kube-exec-stream-refactor branch May 5, 2022 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavannd1 pavannd1 pavannd1 left review comments

@tdmanv tdmanv Awaiting requested review from tdmanv

Assignees
No one assigned
Labels
None yet
Projects
Kanister
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@PrasadG193 @ihcsim @pavannd1 @tdmanv