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

Let's talk configuration! #64

Closed
slifty opened this issue Jun 14, 2020 · 5 comments
Closed

Let's talk configuration! #64

slifty opened this issue Jun 14, 2020 · 5 comments
Labels
discussion The conversation is the point

Comments

@slifty
Copy link
Member

slifty commented Jun 14, 2020

Discussion

What do you want to talk about?

The long term vision for configuration would allow for things like API and UX based configuration.

The short term vision for configuration is necessarily far simpler. This issue is about the short term.

This issue is about fleshing out the contents of a config.json which will allow someone to set up:

  • Ingestion streams
  • A topology of appliances

This list may well expand.

Relevant Resources / Research

None

@slifty slifty added the discussion The conversation is the point label Jun 14, 2020
@slifty
Copy link
Member Author

slifty commented Jun 14, 2020

How's this for a proposed format for the data we want right now:

config.json

{
  "sources": [
    {
      "ingestionEngine": "",
      "settings": {},
      "metadata": {},
    }
  ],
  "appliances": [
    {
      "package": "",
      "name": "",
      "settings": {},
    }
  ]
}

This plan would have some secondary impacts:

  • We might want to create the concept of ingestion engine names (or maybe we would want to just use the class name of the ingestion engine?)
  • We should consider fleshing out the metadata associated with a stream (for now we can just skip that field / not formalize it yet -- we don't have to block on this)
  • Tweak to AbstractIngestionEngine API (see settings note below)

Appliance settings would be up to the individual appliance to specify; the field would just be a key / value store that is passed to the appliance's constructor in the settings parameter.

IngestionEngine settings would probably need to be similar (and we'd have to update our IngestionEngine to take in a settings parameter that is parsed.)

Not included

This does not propose anything related to configuration outside of the immediate implementation path (e.g. some day we might have configuration related to server names, ports, identifying information regarding the TV Kitchen host organization, etc).

Also as alluded to earlier the metadata field related to streams should be much more explicit. For instance appliances are probably going to benefit from knowing the metadata associated with a given stream.

@slifty
Copy link
Member Author

slifty commented Jun 15, 2020

Note: I toyed with the idea of a version as a settings field in appliances.

This would naturally be something we remove depending on where things land with #65.

Really it feels more appropriate to rely on package.json to specify appliance version, and the configuration to simply say which appliance we want (using eslint's api as a model for plugins, which appliances are functionally serving as.)

@reefdog
Copy link
Contributor

reefdog commented Jun 15, 2020

Appliance settings would be up to the individual appliance to specify; the field would just be a key / value store that is passed to the appliance's constructor in the settings parameter.

Strikes me that verifying the format/contents of these settings is another thing the appliance's doctor should do.

Note: I toyed with the idea of a version as a settings field in appliances.

Hard agree with the rest of your comment that this feels like a dangerous anti-pattern that means we're replicating a package manager. I'd say if we find ourselves needing this, we should re-evaluate the path we've chosen. But, I do get that instance configurations / a particular topology may be reliant on a specific version of an appliance. (Like, it might mean that recipes themselves need to be proper apps with package.json files?) Anyway the closer config.json gets to "package.json outside of package.json", the more I squint.

@slifty
Copy link
Member Author

slifty commented Jun 16, 2020

Strikes me that verifying the format/contents of these settings is another thing the appliance's doctor should do.

Doctor would be run outside of the context of configuration of an appliance, so I'm not sure about this.

e.g. doctor would be run (ideally as a triggered part of yarn add) to just say "are all the appliance pre-requisites installed?" -- regardless of whether the appliance is being used in a given recipe (which is where configuration would occur)

I'm not sure settings need a validation method -- if the config is invalid it will either not work as expected OR it will fail to construct because a required setting was not provided. Do other projects with dynamic setting objects have a validation mechanism? I guess travis has travis lint for instance. (either way I think that would be a separate thing, not a doctor thing)

@slifty
Copy link
Member Author

slifty commented Aug 17, 2020

We have moved down a path of using the countertop API to add appliances (see #82)

In particular, configuration is going to happen in TVK instances -- where the countertop is imported as a package (package TBD #80) and coded against, as opposed to configuration happening directly on a clone of this repository.

@slifty slifty closed this as completed Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion The conversation is the point
Projects
None yet
Development

No branches or pull requests

2 participants