-
Notifications
You must be signed in to change notification settings - Fork 704
Use driver feature instead of checking the name #4076
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
There is a chicken-and-egg problem here, where the driver must be provided with a host port on creation... Even though it is ignored, and then replaced with the real address and port as a side-effect of |
Yes, I tried to tackle this in #3901, but was unable to do so due to time constraints. What are the possible ways to tackle this? 🤔 |
Another problem is that the ssh.config is written with the dummy response, so it remains at So while the instance is updated, and |
d3db96a
to
fe6426c
Compare
We will need to fix the issues with "no-op and side-effects" driver API and the address/port allocation separately. |
fe6426c
to
0a3fe5d
Compare
WSL2 CI failing |
Other container drivers with dynamic addresses and static ports also want to set the port after start. Signed-off-by: Anders F Björklund <[email protected]>
type DriverFeatures struct { | ||
CanRunGUI bool `json:"canRunGui,omitempty"` | ||
DynamicSSHAddress bool `json:"dynamicSSHAddress"` | ||
StaticSSHPort bool `json:"staticSSHPort"` |
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.
For consistency with DynamicSSHAddress
, can we invert the logic and make this DynamicSSHPort
?
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.
We could, but then it would be true by default - where we have mostly tried to keep our struct false by default
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.
Since DynamicSSHAddress doesn't really do much at the moment, we could look at revising both of them?
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 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 StaticSSHPort
then.
DynamicSSHAddress
can be revisited in a separate issue
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 will draft a new PR, how we can revise the API (hopefully before 2.0 is out)
Move InspectStatus
to the core, and make SSHAddress
and GetStatus
work
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.
Thanks
Other container drivers with dynamic addresses and static ports also want to set the port after start.
This means that we have to provide their names too in "limatype", instead of making it fully external.
There is some weird hard-coded path handling left, that could be the topic of another PR perhaps?