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

Improve visor config and startup/shutdown logic. #360

Merged

Conversation

evanlinjin
Copy link
Contributor

@evanlinjin evanlinjin commented May 14, 2020

Fixes #339 fixes #173 fixes #170

Changes:

  • Separated app-associated functionality within /pkg/visor into /pkg/app/launcher.
  • Added the concept of visor.BaseConfig() to simplify obtaining default values.
  • Made startup/shutdown logic modular and introduced init and shutdown stacks.
  • Shutdown now uses module-based timeouts and wait groups.

Copy link
Contributor

@Darkren Darkren left a comment

Choose a reason for hiding this comment

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

Really huge improvement. Just some minor queries here and there

cmd/skywire-cli/commands/visor/gen-config.go Show resolved Hide resolved
if !ok {
return ErrAppNotFound
}
if args != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be checking len(args) != 0 imo here. Just to exclude non-nil but still empty args

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Darkren But what if people want to run an app with no args?

pkg/app/launcher/launcher.go Outdated Show resolved Hide resolved
pkg/hypervisor/config.go Outdated Show resolved Hide resolved
pkg/visor/config.go Outdated Show resolved Hide resolved
pkg/visor/config.go Outdated Show resolved Hide resolved
pkg/visor/visor_init.go Outdated Show resolved Hide resolved
志宇 added 3 commits May 15, 2020 17:21
* Separated app-associated functionality within /pkg/visor into /pkg/app/launcher.
* Added the concept of visor.BaseConfig() to simplify obtaining default values.
* Made startup/shutdown logic modular and introduced init and shutdown stacks.
* Shutdown now uses module-based timeouts and wait groups.
@evanlinjin evanlinjin force-pushed the feature/improve-config-#339 branch from 47b6fb0 to 912fe09 Compare May 15, 2020 05:23
Copy link
Contributor

@nkryuchkov nkryuchkov left a comment

Choose a reason for hiding this comment

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

Really great improvements! Good job!

cmd/skywire-cli/commands/visor/app.go Outdated Show resolved Hide resolved
pkg/app/appnet/skywire_networker.go Outdated Show resolved Hide resolved
pkg/hypervisor/hypervisor.go Outdated Show resolved Hide resolved
pkg/visor/config.go Outdated Show resolved Hide resolved
pkg/visor/config.go Outdated Show resolved Hide resolved
pkg/visor/init.go Outdated Show resolved Hide resolved
pkg/visor/rpc.go Outdated Show resolved Hide resolved
pkg/visor/rpc_client.go Outdated Show resolved Hide resolved
pkg/visor/visor.go Outdated Show resolved Hide resolved
pkg/visor/visor.go Outdated Show resolved Hide resolved
@evanlinjin evanlinjin merged commit 0a085f3 into feature/appserver-improvements-#338 May 15, 2020
@jdknives jdknives deleted the feature/improve-config-#339 branch June 15, 2020 17:37
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.

3 participants