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

Extend app hosting to cover Seq requirements #139

Merged
merged 7 commits into from
Jul 31, 2020

Conversation

nblumhardt
Copy link
Member

@nblumhardt nblumhardt commented Jul 29, 2020

Seq currently ships Seq.Apps.GenericHost and Seq.Apps.Interrogator binaries for working with .NET app packages. These two executables load and execute app packages, and read app package metadata.

In a near-future Seq release, we'd like to replace GenericHost and Interrogator with corresponding commands from seqcli. The app run command already implements the generic hosting functionality, with some minor feature gaps.

By closing those gaps, and adding a new seqcli app define command to play the role of Interrogator, we can remove the two standalone executables.

This will:

  • Reduce the sizes of the Windows installer and Docker image
  • Make the development-time app debugging infrastructure (seqcli app run) match the runtime app environment exactly, making behavior around dependent assembly resolution and binding more predictable

There are three parts to this work:

  1. This PR: introduce the required commands and features in seqcli
  2. A near-future Seq release: let users opt-in to running apps with the new host
  3. A Seq release on .NET 5, which will switch all app hosting to the new model, breaking Seq's last .NET Framework dependencies on Windows

Before this can be merged:

  • Implement seqcli app define, to read app metadata from .NET attributes
  • Add code-level documentation for the app metadata format should be covered in docs
  • Add tests for app define
  • Allow app run to consume settings from environment variables, so that command-line length limits are avoided, and app configuration can trivially include newlines and other non-CLI-safe characters
  • Set the app's id and title correctly in app run
  • Migrate any remaining tests from the Seq codebase that cover the app loader, host, and interrogator

@nblumhardt nblumhardt added the enhancement New feature or request label Jul 29, 2020
@nblumhardt nblumhardt marked this pull request as draft July 29, 2020 03:07
@nblumhardt nblumhardt marked this pull request as ready for review July 30, 2020 08:42
Capabilities = GetCapabilities(seqAppType),
Platform = new Dictionary<string, AppPlatformDefinition>
{
["hosted-dotnet"] = new AppPlatformDefinition
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 replaces the __MainReactorTypeName top-level property; reasonable to include here because hosted-dotnet is a logical alternative for other platform values (win-x64 etc.)

Copy link
Member

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

Looks good to me (once the build is 🍏)!

@nblumhardt nblumhardt merged commit 090743c into datalust:dev Jul 31, 2020
@nblumhardt nblumhardt mentioned this pull request Aug 18, 2020
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

Successfully merging this pull request may close these issues.

2 participants