Skip to content
This repository was archived by the owner on Feb 16, 2023. It is now read-only.

Refactor UI package#277

Merged
jpcoenen merged 42 commits intofeature/audit-log-bash-paginationfrom
fix/build-windows
May 18, 2020
Merged

Refactor UI package#277
jpcoenen merged 42 commits intofeature/audit-log-bash-paginationfrom
fix/build-windows

Conversation

@mackenbach
Copy link
Copy Markdown
Member

@mackenbach mackenbach commented Apr 24, 2020

This started out as a fix for the build problems introduced in #274, but ended up as a refactor of the ui package.

The main changes:

  • ui.IO interface changed from:
type IO interface {
	Stdin() io.Reader
	Stdout() io.Writer
	Prompts() (io.Reader, io.Writer, error)
	IsStdinPiped() bool
	IsStdoutPiped() bool
}

to:

type IO interface {
	// Input returns an io,Reader that reads input for the current process.
	// If the process's input is piped, this reads from the pipe otherwise it asks input from the user.
	Input() io.Reader
	// Output returns an io.Writer that writes output for the current process.
	// If the process's output is piped, this writes to the pipe otherwise it prints to the terminal.
	Output() io.Writer
	// Stdin returns the *os.File of the current process's stdin stream.
	Stdin() *os.File
	// Stdin returns the *os.File of the current process's stdout stream.
	Stdout() *os.File
	// Prompts returns an io.Reader and io.Writer that read and write directly to/from the terminal, even if the
	// input or output of the current process is piped.
	// If this is not supported, an error is returned.
	Prompts() (io.Reader, io.Writer, error)
	// ReadSecret reads a line of input from the terminal while hiding the entered characters.
	// Returns an error if secret input is not supported.
	ReadSecret() ([]byte, error)
	// IsInputPiped returns whether the current process's input is piped from another process.
	IsInputPiped() bool
	// IsOutputPiped returns whether the current process's output is piped to another process.
	IsOutputPiped() bool
}

This allows users of the interface to explicitly request the underlying stdin or stdout file when they need it (for example, when spawning a child process). Also the naming of IsInputPiped and IsOutputPiped better describes the interface than IsStdinPiped and IsStdoutPiped.

  • The interface is actually implemented by different structs (standardIO, windowsIO and ttyIO) instead of a single struct with different fields set.

This replaces the changes from #274.

SimonBarendse and others added 12 commits February 27, 2020 15:34
It was leftover from the ui package refactor.
It's no longer needed now that the isPiped logic is moved to a
helper function.
They aren't used outside the package and have accessor functions
so there's no need for them to be public.
This should display the jobs as verify-build-<os>-<arch> instead of verify-build-<arch>-<os>.
@jpcoenen
Copy link
Copy Markdown
Member

jpcoenen commented Apr 24, 2020

Build verification is introduced in #278.

It is weird to reuse the same struct for different implementations of an interface, while it is also possible to give them all a separate struct. This is way cleaner.
These names better describe the abstraction. Stdin and Stdout describe the implementation, not the interface.
This allows direct interfacing with stdin and stdout for some specific use-cases.
This has not been used for a long time and it has some compatibility issues with the ui update. So we can just as well delete it.
@jpcoenen jpcoenen marked this pull request as ready for review April 24, 2020 15:38
@jpcoenen
Copy link
Copy Markdown
Member

I think I fixed most of the issues. Could use some manual testing to verify whether it actually works on all platforms.

@SimonBarendse
Copy link
Copy Markdown
Member

I believe #274 was also introduced to fix a bug, there is a package (I believe in the standard library) that changes its behavior based on the passed Writer being os.Stdout. Before the refactor not os.Stdout itself was returned, but a wrapped version. The refactor changed this so that os.Stdout would be detected as stdout by the package.

This shows the importance of descriptive and complete PR messages.

@Marton6 Do you remember which bug and package this was about exactly?

Comment thread internals/cli/ui/io_unix.go Outdated
@jpcoenen jpcoenen requested a review from SimonBarendse April 30, 2020 08:11
@SimonBarendse SimonBarendse added the bug Something isn't working label Apr 30, 2020
Copy link
Copy Markdown
Member

@SimonBarendse SimonBarendse left a comment

Choose a reason for hiding this comment

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

It's not clear to me which changes are required to fix the build and which are just refactors. I hope the questions I asked below clear things up, if not, let's have a quick call.

Comment thread internals/cli/ui/io_unix.go Outdated
Comment thread internals/cli/ui/io_windows.go
Comment thread internals/cli/ui/io.go Outdated
Comment thread internals/cli/ui/io.go
jpcoenen added 4 commits May 4, 2020 12:06
The exec package checks if an os.File is passed. So we should explicitly set Stdout and not a wrapped writer.
These names better describe the abstraction. Stdin and Stdout are implementations of input and output. The interface should not describe the implementation.
jpcoenen added 3 commits May 4, 2020 14:55
This allows us to use t.Cleanup()
The command ran by secrethub run writes to Stdout() instead of Output(). The test should reflect that to succeed.
Comment thread .circleci/config.yml
@jpcoenen jpcoenen removed the bug Something isn't working label May 6, 2020
@jpcoenen
Copy link
Copy Markdown
Member

jpcoenen commented May 6, 2020

Removed the bug label because the PR that introduced this has been reverted. (#279)

Changeset itself is still relevant.

@jpcoenen jpcoenen changed the title Fix build on windows Refactor UI package May 7, 2020
jpcoenen added 2 commits May 8, 2020 14:57
This makes it possible to mock this functionality in tests.
@jpcoenen jpcoenen changed the base branch from develop to feature/audit-log-bash-pagination May 18, 2020 07:31
@jpcoenen
Copy link
Copy Markdown
Member

Since this is not really useful on itself, I am merging it into #269.

@jpcoenen jpcoenen force-pushed the fix/build-windows branch from d6ae37b to e6ed6df Compare May 18, 2020 07:58
@mackenbach
Copy link
Copy Markdown
Member Author

@jpcoenen feel free to merge when you feel it's ready

Comment thread .circleci/config.yml
@jpcoenen jpcoenen merged commit 0c8ae41 into feature/audit-log-bash-pagination May 18, 2020
@jpcoenen jpcoenen deleted the fix/build-windows branch May 18, 2020 09:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants