Skip to content
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

refactor lib/sshnp.dart to allow it to be used in a desktop app as well as a CLI #248

Closed
gkc opened this issue Jul 11, 2023 · 5 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@gkc
Copy link
Contributor

gkc commented Jul 11, 2023

Is your feature request related to a problem? Please describe.

Currently there are some behaviours which will not work in a desktop app. For example exit statements

Describe the solution you'd like

SSHNP class should be reusable in either the existing CLI or in the (new) desktop app.

Describe alternatives you've considered

No response

Additional context

No response

@gkc gkc added the enhancement New feature or request label Jul 11, 2023
@gkc gkc self-assigned this Jul 11, 2023
@XavierChanth
Copy link
Member

I will start this in tandem with the changes I'm doing for #196

@XavierChanth XavierChanth self-assigned this Jul 12, 2023
@XavierChanth
Copy link
Member

XavierChanth commented Jul 12, 2023

@gkc I noticed that the sessionId was not being used to generate the path in sshnpd...
https://github.com/atsign-foundation/sshnoports/blob/284b01cb14012a593d39b2a37edfd960fed48708/packages/sshnoports/lib/sshnpd.dart#L296-L314

but it is used in sshnp...

https://github.com/atsign-foundation/sshnoports/blob/284b01cb14012a593d39b2a37edfd960fed48708/packages/sshnoports/lib/sshnp.dart#L600-L618

I am trying to merge these two functions into a single one, since they are similar enough.
Should sshnpd's path also be '$atsign/$sessionId' for its hive path or should I make sessionId an optional part of the path like in my code below:

Future<AtClient> createAtClientCli(
    {required String homeDirectory,
    required String atsign,
    String? sessionId,                    // ! OPTIONAL
    required String atKeysFilePath}) async {
  
  String pathBase = '$homeDirectory/.sshnp/$atsign/';
  if (sessionId != null) {
    pathBase += '$sessionId/';            // ! ONLY ADDED WHEN NOT NULL
  }
  // ... rest of code
}

@XavierChanth
Copy link
Member

Starting my work here:

I separated the Impl of SSHNPD to SSHNPDImpl

https://github.com/atsign-foundation/sshnoports/tree/xc-sshnp-refactor

There are technically breaking changes... but only to anyone depending on sshnoports as a library... I don't think we should let that stop us from making these improvements though. Especially since the binaries themselves will be non-breaking

@gkc
Copy link
Contributor Author

gkc commented Jul 12, 2023

@XavierChanth they are different for sshnpd vs sshnp because the storage path acts as a kind of a mutex. We want only one sshnpd to be running at any given time for a given user on a given host or container; but we want to allow many sshnp to be run concurrently

@cconstab can explain further

FYI I'm out of office until Monday now so I'll be catching up with work stuff once or twice a day Thu & Fri but not at all on Sat & Sun

@XavierChanth
Copy link
Member

Closed by the latest refactoring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants