-
Notifications
You must be signed in to change notification settings - Fork 73
feat(goose): Add tmux support and Session Name variable to Goose #113
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
Conversation
- Introduced new variables `experiment_use_tmux` and `session_name` for configuring tmux or screen sessions. - Updated script logic to handle both tmux and screen, ensuring only one can be enabled at a time. - Enhanced error handling for missing dependencies and session management for both tmux and screen.
…n and session name variable
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.
Could you also move the script to a run.sh? It could also be a separate PR effort. Thanks
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.
Yeah I'll go ahead split it into the run script shouldn't be too hard
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.
The Terraform looks fine to me. I'm just always a little more skittish about approving Bash – my Bash skills are lacking
experiment_report_tasks = true | ||
# Run Goose in the background | ||
# Run Goose in the background with screen (pick one: screen or tmux) |
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.
From a data modeling standpoint, would we want to get rid of the screen and TMUX variables, and have a single string variable that has the value of "tmux"
or "screen"
(using a validation field to make sure the value is one of those two)?
I don't know what that would mean for how we've handled versions up until now, since that would be a breaking change, but it feels janky to me that a user can accidentally set both of them, even though only one should be set at a time. I know the script checks for whether they're both set, but for user clarity, it feels like the data should be modeled as being more mutually exclusive
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.
Honestly that's a super valid point that would be a much simpler solution
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.
Let's keep the variables for now.
We can make this change for all modules at some time and bump to 2.0.0
I pretty much have finished splitting the coder_app logic into the run.sh, as well as unifying the variable for screen and tmux with validation. Since this is a breaking change like Parkreiner mentioned, what would you all prefer I do. Would it be a big deal to push breaking changes in this commit in regards to tmux support or how would you wanna handle this? |
Tested in dev environment.
Reference: #31