RFC: Include CLI packaging information within the connector metadata #322
Conversation
| nativeToolchainDefinition?: NativeToolchainDefinition; | ||
| supportedEnvironmentVariables: EnvironmentVariableDefinition[]; | ||
| commands: Commands; | ||
| cliPlugin?: BinaryCliPluginDefinition | DockerCliPluginDefinition; |
There was a problem hiding this comment.
This new type for cliPlugin is a breaking change to the metadata format. We should avoid breaking changes. Can this be expressed in a non-breaking way?
There was a problem hiding this comment.
Yes, but unfortunately it's still a breaking change to that type because:
BinaryCliPluginDefinitionis missing a version field (version: string)BinaryCliPluginDefinition's type property should be optional (ietype?: "Binary") (because before this was a union, there was no type property)
Also, what does it mean for a BinaryCliPluginDefinition to have a version and also platforms? Perhaps it would be better to separate the old BinaryCliPluginDefinition (which means "get it from the cli plugin index") from the new definition by creating a new type BinaryInlineCliPluginDefinition which has platforms but no version (and type: "BinaryInline") so that way its very clear that one can either have a version or platforms but not both. And maybe you don't even need name in BinaryInlineCliPluginDefinition?
There was a problem hiding this comment.
Also, what does it mean for a BinaryCliPluginDefinition to have a version and also platforms? Perhaps it would be better to separate the old BinaryCliPluginDefinition (which means "get it from the cli plugin index") from the new definition by creating a new type BinaryInlineCliPluginDefinition which has platforms but no version (and type: "BinaryInline") so that way it is very clear that one can either have a version or platforms but not both. And maybe you don't even need a name in BinaryInlineCliPluginDefinition?
That's a great idea @daniel-chambers! Using your suggestion, I can have a validation in place for the new connector versions during the connector publishing automation, to only have the BinaryInlineCliPluginDefinition as we are going to sunset the use of the cli plugins index for the ddn connectors eventually.
There was a problem hiding this comment.
That sounds good @codingkarthik. Keep in mind that we'll need to support BinaryCliPluginDefinition for quite some time until the connector versions that use it (ie. all of them right now) are no longer used by users.
There was a problem hiding this comment.
@SandeepSamba As per our discussion, the name and the version of the inlined binary CLI plugin definition will be the same as the connector name and version, and in case the connector wants to use a different version of the Binary CLI plugin, they will have to specify the name and the version of the Binary CLI plugin they want to use.
cc: @daniel-chambers @scriptnull
| */ | ||
| bin: string; | ||
|
|
||
| files: FilePath[]; |
There was a problem hiding this comment.
What is files for and how does it differ/relate to bin, uri, sha256? Might deserve some comments around this and the FilePath type.
I know this is basically a copy/paste of what's in the plugin index, but it seems weird to me to have an array of files here but really only support one file, since there's only one uri, bin and sha256...
There was a problem hiding this comment.
@SandeepSamba Could you please help out here? I'm not sure what the files field meant in the CLI plugin manifest which is why I left it empty here.
There was a problem hiding this comment.
This is an example of your BinaryCliPluginPlatform type. files from and to is basically the name of the binary that is going to get downloaded from the url and what it should be renamed to. CLI does all of this right now and there probably is a good scope to loosen this weird interface
There was a problem hiding this comment.
Yeah, so why does it need to be a list? Feels like the whole files thing is redundant, you could just rename the file downloaded from uri to the filename in bin. Am I missing something?
There was a problem hiding this comment.
I agree. I don't know the use case it was introduced for but as I said we can greatly improve this entire thing.
There was a problem hiding this comment.
Folks, then I will go ahead and remove the files thing.
There was a problem hiding this comment.
@codingkarthik files is needed if we have to ship some supporting files to the plugin (For eg. We need to provide some .dll files for the windows binary to work correctly, supporting header files for a plugin which is not statically linked. Also think of a use case where the bin file (the file that is invoked by the CLI) is a .bat or a .sh file and it calls multiple binaries inside them). Without files then all we can do is to unzip the entire archive and place everything as it was in the archive in the plugins directory. from and to is to support renaming when the plugin is being unpacked and installed (Can also be used to change the location of the file via renaming). We can remove the files field, but then we lose out on these flexibilities. cc @daniel-chambers
There was a problem hiding this comment.
Honestly, if it were me, I'd just require every plugin to be a .tar.gz (or zip, or whatever archive format you want) and require connectors to publish their files with the correct filenames inside that archive. Why have the complexity of having a custom file renaming system (files) and requiring connectors to define the renames here and the CLI to implement understanding it and do file renaming? We have technology already that represent files with names potentially in directories, wrapped up in a single file: tar/zip files. We should just use it.
Add Binary CLI Plugin Packaging Information to
connector-metadata.yamlThis PR introduces changes to improve the management of binary CLI plugin information in the
connector-metadata.yamlfile.Key Changes
connector-metadata.jsonto include binary CLI plugin packaging information.versionfield to theConnectorMetadataDefinitiontype for future versioning.BinaryCliPluginDefinitionto accommodate inline binary CLI definition.Addition of
connector-metadata-typesA significant part of this PR is the introduction of
connector-metadata-types. This file contains TypeScript type definitions for theconnector-metadata.jsonstructure and is meant to act as the source of truth for the schema of theconnector-metadata.jsonfile. As we advance, it is expected that if any changes are proposed in the connector metadata structure, they also update theconnectorTypes.tsfile in theconnector-metadata-typesfolder.