Skip to content

Fix: Pass SSHLocalPort in Instance to configure driver #3746

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix: Pass SSHLocalPort in Instance to configure driver #3746

wants to merge 1 commit into from

Conversation

unsuman
Copy link
Contributor

@unsuman unsuman commented Jul 17, 2025 •
edited
Loading

This pull request refactors the configuration mechanism for drivers in the pkg/driver package, by passing the SSHLocalPort directly through Instance struct.

Closes #3703

@unsuman unsuman force-pushed the fix/rm-sshlocalport branch 4 times, most recently from 006b8ab to 4ccf7d9 Compare July 17, 2025 17:11
afbjorklund
afbjorklund requested changes Jul 17, 2025
View reviewed changes
Copy link
Member

@afbjorklund afbjorklund left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I only meant to wrap the "int" second parameter in a struct, I think the first parameter was good as it was.

The name can be discussed, if it is ConfigOption or RuntimeOption or whatever it is actually referring to

pkg/driver/driver.go Outdated
// SetConfig sets the configuration for the instance.
Configure(inst *store.Instance, sshLocalPort int) *ConfiguredDriver
// Configure sets the configuration for the instance.
Configure(opts ...ConfigOption) *ConfiguredDriver
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to keep the instance, since that will always be there.

Configure(inst *store.Instance, opts ...ConfigOption) *ConfiguredDriver

@afbjorklund afbjorklund requested a review from AkihiroSuda July 17, 2025 18:23
afbjorklund
afbjorklund reviewed Jul 17, 2025
View reviewed changes
pkg/driver/external/driver.proto Outdated
@@ -41,8 +41,7 @@ message StartResponse {
}

message SetConfigRequest {
bytes instance_config_json = 1;
int64 ssh_local_port = 3;
bytes config_opts = 1;
Copy link
Member

Choose a reason for hiding this comment

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

might have to decide on a transport here

afbjorklund
afbjorklund reviewed Jul 17, 2025
View reviewed changes
pkg/driver/external/driver.proto
@@ -41,8 +41,7 @@ message StartResponse {
}

message SetConfigRequest {
bytes instance_config_json = 1;
Copy link
Member

Choose a reason for hiding this comment

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

this one should remain, I think?

@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone Jul 17, 2025
@AkihiroSuda AkihiroSuda added the gsoc/2025 Google Summer of Code 2025 label Jul 17, 2025
@unsuman unsuman force-pushed the fix/rm-sshlocalport branch from 4ccf7d9 to b60a4c2 Compare July 18, 2025 10:14
AkihiroSuda
AkihiroSuda reviewed Jul 18, 2025
View reviewed changes
pkg/driver/external/driver.proto Outdated
@@ -42,7 +42,7 @@ message StartResponse {

message SetConfigRequest {
bytes instance_config_json = 1;
int64 ssh_local_port = 3;
bytes config_opts = 2;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need two jsons ?
Can't we just set the SSH port in the instance_config_json ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to set it in Instance.SSHLocalPort?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then, is there a need for a ConfigOption?

Copy link
Member

Choose a reason for hiding this comment

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

Probably no

Copy link
Member

Choose a reason for hiding this comment

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

I thought the lima.yaml (config) said "0" for the port, and then the other parameter passed the current random port?

But if we edit the instance config in the store each time, then we don't need any runtime parameters

@unsuman unsuman force-pushed the fix/rm-sshlocalport branch from b60a4c2 to 9ea8cda Compare July 18, 2025 16:53
@unsuman unsuman changed the title Fix: Pass SSHLocalPort as an option to configure driver Fix: Pass SSHLocalPort in Instance to configure driver Jul 18, 2025
@unsuman
set sshlocalport to instance
a680340 
Signed-off-by: Ansuman Sahoo <[email protected]>
@unsuman unsuman force-pushed the fix/rm-sshlocalport branch from 9ea8cda to a680340 Compare July 18, 2025 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@afbjorklund afbjorklund afbjorklund requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
gsoc/2025 Google Summer of Code 2025
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.

SSHLocalPort should be in instance, and not as a parameter
3 participants
@unsuman @AkihiroSuda @afbjorklund