-
Notifications
You must be signed in to change notification settings - Fork 380
Tool gotty #2583
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
Tool gotty #2583
Conversation
|
Still need to add the robot tests and documentation... @steiler I have some functions to be shared between sshx and gotty, is cmd/common/tools.go a "good" place or should it be in utils? |
|
You might want to take a look at this Line 226 in 0f03a98
Parts of what you're trying here is already implemented here GetOwner I think is more of a utils function ... it always depends... is it general util usefull in general or not. |
cmd/common/tools.go
Outdated
| var err error | ||
|
|
||
| // If lab name is provided directly, use it | ||
| if toolLabName != "" { |
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.
why don't you rename the function var name to labName then you can spare this if
cmd/common/tools.go
Outdated
| } | ||
|
|
||
| // If topo file is provided or discovered | ||
| if Topo == "" && labName == "" { |
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.
there is a WithTopoPath option, that should handle all of this.
Line 211 in 0f03a98
| return func(c *CLab) error { |
cmd/common/tools.go
Outdated
| } | ||
|
|
||
| // If we have lab name but no topo file, try to find it from containers | ||
| if labName != "" && Topo == "" { |
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.
Thinking about it, this should probably also become a ClabOption itself, then it would by a single switch that initializes one or the other ClabOption.
|
@steiler I tried to solve your remarks. From my side this is ready |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2583 +/- ##
==========================================
+ Coverage 54.38% 54.91% +0.53%
==========================================
Files 181 184 +3
Lines 19695 20149 +454
==========================================
+ Hits 10711 11065 +354
- Misses 7874 7944 +70
- Partials 1110 1140 +30
🚀 New features to boost your workflow:
|
Similar to containerlab tools sshx, this as add a web ui, but instead of web, this is fully local with password protection.