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: noports core #507

Merged
merged 52 commits into from
Oct 5, 2023
Merged

refactor: noports core #507

merged 52 commits into from
Oct 5, 2023

Conversation

XavierChanth
Copy link
Member

@XavierChanth XavierChanth commented Sep 28, 2023

- What I did

CI:

  • Added the ability to test using --legacy-daemon in the end2end test suite
    • Set the reverse and sshnpd installer tests to legacy daemon
  • Added tag verification composite actions for both noports_core and sshnoports
    • reimplemented these checks where they were previously hardcoded
    • Also added unit tests to check these tags as well

Docker

  • Changed the context for all images to be the root of the repo
    • Allows building from the local copy of noports_core when necessary

noports_core:

  • Moved all of lib/ into lib/src/ so that the code is protected.
    • Reexported all of the code into separate libraries:
      • sshnp
      • sshnp_params - this has a lot of addons, so only the bare minimum is included in sshnp
      • sshnpd
      • sshrv
      • sshrvd
      • utils
  • Wrapped all default args as static const vars in DefaultArgs and all SSHNP only ones in DefaultSSHNPArgs
  • Added 2 new layers of abstraction to SSHNP:
    • followed the appropriate code paths to ensure that all of the same logic runs in the same order as before.
classDiagram
    SSHNP <|-- SSHNPImpl
    SSHNPImpl <|-- SSHNPForwardDirection
    SSHNPImpl <|-- SSHNPReverseDirection
    SSHNPForwardDirection <|-- SSHNPForwardDartImpl
    SSHNPForwardDirection <|-- SSHNPForwardExecImpl
    SSHNPReverseDirection <|-- SSHNPReverseImpl
    SSHNPReverseDirection <|-- SSHNPLegacyImpl
    class SSHNP{
        <<abstract interface>>
      Exports public API
    }
    class SSHNPImpl{
        <<abstract>>
      Implements common code between all 4 implementations 
    }
    class SSHNPForwardDirection{
      <<mixin>>
      Implements common forward code
    }
    class SSHNPReverseDirection{
      <<mixin>>
      Implements common reverse code
    }
    class SSHNPForwardDartImpl {
        forward pure-dart client
    }
    class SSHNPForwardExecImpl {
        forward ssh exec client
    }
    class SSHNPReverseImpl {
        reverse ssh client for non-sshrvd hosts
    }
    class SSHNPLegacyImpl {
        reverse ssh client for <4.0.0 daemons
    }
Loading
  • Instead of using boolean for checking if initialization has occurred, there is now a completer which completes when initialization is done. By default, SSHNP automatically schedules initialization to begin in the constructor.
    • An internal boolean is still stored, which prevents successive init calls from running.
  • Args:
    • Added --help to print usage
    • Moved createArgParser into the SSHNPArg class as a static function
    • Made mandatory arguments in SSHNPParams to be final String instead of final String?
  • Rewrote SSHNPResult
classDiagram
    SSHNPResult <|-- SSHNPSuccess
    SSHNPResult <|-- SSHNPFailure
    SSHNPResult <|-- SSHNPConnectionBean
    SSHNPFailure <|-- SSHNPError
    Exception <|-- SSHNPError
    SSHNPSuccess <|-- SSHNPNoOpSuccess
    SSHNPSuccess <|-- SSHNPCommand
    SSHNPConnectionBean <|-- SSHNPCommand
    class SSHNPConnectionBean {
        <<mixin>>
    }
    class SSHNPNoOpSuccess {
      Needed for pure dart since it doesn't output a command
    }
Loading
  • noports_core now has it's own version.dart (this is the version.dart that will be used to report version to the daemon)

- How I did it

- How to verify it
N.B. the trunk tests are expected to fail because the branch doesn't have the updated code to build sshnoports with the local noports_core dependency yet (this PR will fix that)

- Description for the changelog
refactor: noports core

@XavierChanth
Copy link
Member Author

Running the e2e tests against 55a5ad2

Which uses the already published noports_core 4.0.0-dev.2 ~ the end2end tests aren't equipped to handle building and using noports_core from local yet.

@XavierChanth XavierChanth changed the title refactor: noports core [DRAFT] refactor: noports core Sep 29, 2023
@XavierChanth XavierChanth changed the title [DRAFT] refactor: noports core refactor(DRAFT): noports core Sep 29, 2023
@XavierChanth
Copy link
Member Author

6407b66 - I was being over strict with SDK constraints, it doesn't need to be enforced there, I just did it because we had no reason not to... until now...

@@ -102,7 +102,7 @@ class _ProfileFormState extends ConsumerState<ProfileForm> {
crossAxisAlignment: CrossAxisAlignment.start,
children: [
CustomTextFormField(
initialValue: oldConfig.sshnpdAtSign ?? '',
initialValue: oldConfig.sshnpdAtSign,
Copy link
Member Author

Choose a reason for hiding this comment

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

these were changed from String? to String

legacy_sub='s/legacy/--no-legacy-daemon/g'
fi
eval "$prefix" "$legacy_sub" ../sshnp/entrypoint.sh
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is where we insert the --legacy-daemon flag into the command.

@@ -1,6 +1,6 @@
#!/bin/bash
echo "SSHNP START ENTRY"
SSHNP_COMMAND="$HOME/.local/bin/sshnp -f @sshnpatsign -t @sshnpdatsign -d deviceName -h @sshrvdatsign -s id_ed25519.pub -v > sshnp.log"
SSHNP_COMMAND="$HOME/.local/bin/sshnp -f @sshnpatsign -t @sshnpdatsign -d deviceName -h @sshrvdatsign -s id_ed25519.pub -v legacy > sshnp.log"
Copy link
Member Author

Choose a reason for hiding this comment

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

legacy is the placeholder where the --legacy-daemon flag might be inserted

dart compile exe ${REPO_DIR}/packages/sshnoports/bin/sshrvd.dart -o ${OUTPUT_DIR}/sshrvd ; \
dart compile exe ${REPO_DIR}/packages/sshnoports/bin/activate_cli.dart -o ${OUTPUT_DIR}/at_activate ;
dart pub get; \
dart run melos bootstrap --scope="noports_core" --scope="sshnoports"; \
Copy link
Member Author

Choose a reason for hiding this comment

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

This step is responsible for doing local linking of sshnoports and noports_core

dart compile exe ${REPO_DIR}/packages/sshnoports/bin/activate_cli.dart -o ${OUTPUT_DIR}/at_activate ;
dart pub get; \
dart run melos bootstrap --scope="noports_core" --scope="sshnoports"; \
dart pub get -C ${PACKAGE_DIR}; \
Copy link
Member Author

Choose a reason for hiding this comment

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

Then once linked, doing pub get will cause sshnoports to pull from noports_core locally

dart compile exe ${REPO_DIR}/packages/sshnoports/bin/activate_cli.dart -o ${OUTPUT_DIR}/at_activate ;
cd ${REPO_DIR}; \
dart pub get; \
dart run melos bootstrap --scope="noports_core" --scope="sshnoports"; \
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above

}

mv "$SRC_DIR/pubspec_overrides.yaml" "$SRC_DIR/pubspec_overrides.back.yaml"
eval "$DART pub get -C $SRC_DIR" || restore_backup_and_exit 1
Copy link
Member Author

@XavierChanth XavierChanth Oct 3, 2023

Choose a reason for hiding this comment

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

This prevents accidentally packaging for apple-silicon using the local noports_core instead of the published one if you've run melos bootstrap on your system.

@XavierChanth XavierChanth self-assigned this Oct 3, 2023
@XavierChanth XavierChanth changed the title refactor(DRAFT): noports core refactor: noports core Oct 3, 2023
@XavierChanth XavierChanth marked this pull request as ready for review October 3, 2023 01:04
Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

@XavierChanth Thanks for this! There's a lot happening here... it all looks good to me but I note that the e2e tests did not run (for the most recent push at least) ... what's the plan for testing?

@XavierChanth
Copy link
Member Author

XavierChanth commented Oct 4, 2023

@XavierChanth Thanks for this! There's a lot happening here... it all looks good to me but I note that the e2e tests did not run (for the most recent push at least) ... what's the plan for testing?

Tests should run now, there was a merge conflict in the pubspec.lock

Now that we have some code isolation we can start unit testing, I have a second PR coming soon which will enable mobile app support and might make some changes to the work I've done here

Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

e2e tests failing

@XavierChanth
Copy link
Member Author

e2e tests failing

trunk tests will fail until this is merged, because trunk doesn't know how to build noports_core (but noports_core is already merged)

@gkc
Copy link
Contributor

gkc commented Oct 5, 2023

Oh I see - ok

@XavierChanth XavierChanth merged commit 4cb9e04 into trunk Oct 5, 2023
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants