Move closehandler to pkg, move feedback form to closehandler #72
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Morishiri
reviewed
Dec 10, 2020
| @@ -29,8 +29,6 @@ var rootCmd = &cobra.Command{ | |||
| func init() { | |||
| cobra.OnInitialize(initLogger) | |||
|
|
|||
| displayOptions.FeedbackFormURL = "https://forms.gle/K9ga7FZB3deaffnV7" | |||
There was a problem hiding this comment.
Would that be possible to leave it here and pass displayOptions to closehandler?
Morishiri
reviewed
Dec 14, 2020
internal/app/loophole/loophole.go
Outdated
| func ForwardPort(config lm.ExposeHttpConfig) { | ||
| setupCloseHandler(config.Display.FeedbackFormURL) | ||
| closehandler.SetupCloseHandler() |
There was a problem hiding this comment.
what if you would set it up in the commands only? E.g. in each relevant Run method, or even PreRun? I think it should then not duplicate the output and should be enough to fix ctrl c breaking temrinal
There was a problem hiding this comment.
I don't remember why I thought that it needs to be setup in every command separately, after some testing I just moved it to the init of the root command and it worked for both ssh and basic auth.
Morishiri
reviewed
Dec 15, 2020
| } | ||
|
|
||
| //SuccessfulConnectionOccured sets the corresponding boolean to true, enabling the display of the feedback form URL after closing the CLI | ||
| func SuccessfulConnectionOccured() { |
There was a problem hiding this comment.
maybe MarkConnectionSuccessful instead?
Morishiri
approved these changes
Dec 15, 2020
Morishiri
force-pushed
the
fix/invisible-terminals-in-pkgs
branch
from
dac5642
to
9052e3d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #71
There is still a bug where several instances of "Goodbye" will be printed at the end because a separate closehandler seems to be needed for every pkg inside of which the user might press CTRL+C; a way of closing the closehandler without closing the program is the next step