-
Notifications
You must be signed in to change notification settings - Fork 582
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.
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
[Refactor] refactor image save flagging process #1924
Conversation
e956022
to
ec9107a
Compare
ec9107a
to
57c05ec
Compare
if options.Output != "" { | ||
f, err := os.OpenFile(options.Output, os.O_CREATE|os.O_WRONLY, 0644) | ||
if err != nil { | ||
return err | ||
} | ||
defer f.Close() | ||
options.Stdout = f | ||
} else { | ||
if isatty.IsTerminal(os.Stdout.Fd()) { | ||
return fmt.Errorf("cowardly refusing to save to a terminal. Use the -o flag or redirect") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(below is more for discussion and see other reviewers' thought).
I think we should remove Output string
from ImageSaveOptions
. Functionally, it represents the same thing as Stdout io.Writer
and having both might cause inconsistency and confusing, for example:
- Even
Output != ""
, we initStdout = cmd.OutOrStdout()
when creatingImageSaveOptions
, and then change it after the file writer created (i.e., here). - Should a caller specify
Output
, orStdout io.Writer
?
If we remove Output
and keep Stdout
only:
pkg/cmd
side logic doesn't need to know where the writer comes from. It just writes content to it.- On
cmd
side, we initopt.Stdout = cmd.OutOrStdout()
ifoutput == ""
or create a file writer and use it asopt.Stdout
(i.e., this pard of code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your thought is a pretty good idea. I will keep the code quo for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL. Thanks. @AkihiroSuda @Zheaoli
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to make another PR to simplify the logic. I prefer to keep the same code here (one PR for one thing). BTW I have made a new issue to discuss this
Signed-off-by: suyanhanx <suyanhanx@gmail.com>
57c05ec
to
aee0d8a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thanks
part of #1680
Checklist:
pkg/api/types/${cmd}_types.go
, and define the CommandOption for this commandpkg/cmd/${cmd}
, and move the command entry point in real into this packageSigned-off-by: suyanhanx suyanhanx@gmail.com