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

Distribution RFC (Milestone 1) #107

Merged
merged 6 commits into from
Mar 12, 2024
Merged

Conversation

sordina
Copy link
Contributor

@sordina sordina commented Feb 23, 2024

RFC for distribution aspects of connectors.

Related specifications:

While the precursor specifications outline the structure and mechanisms of packaging, this RFC outlines the initial distribution of package definitions.

See the Rendered Version of the RFC here.

@sordina sordina changed the title distribution-rfc-milestone-1 Distribution RFC (Milestone 1) Feb 23, 2024
Comment on lines 50 to 52
"source": {
"hash": "98801634b0e1396c933188eef88178952f412a8c",
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you please describe the use of this stanza for me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. I'll add clarification in the RFC, but basically the source link can reference any field in the sources stanza in order to show the provenance of a definition.

Comment on lines +29 to +30
* Add link and checksum to package definition archive to DB Schema
* Add link and checksum to package definition archive to Github metadata format
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if I understand this fully. Do you mean the git tag references in our current DB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This references updating the DB/schmea to be able to store and fetch the new packages stanza. Not sure of the changes required to do this.

"checksum": {
"type": "sha256",
"value": "9283dh9283u...hd092ujdf2ued"
},
Copy link
Member

@scriptnull scriptnull Feb 26, 2024

Choose a reason for hiding this comment

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

we will probably need the information of whether a version of connector is of type PreBuiltDockerImage or ManagedDockerBuild.


The database backing the API provides all of the APIs state management capabilities outside of package archive storage (as described in "Storage").

The initial implementation of the Database will be an extension of the exiting hub registry Postgres instance.
Copy link
Member

Choose a reason for hiding this comment

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

correct! For the beta release, we have created an entirely new schema in the existing postgres DB that could be used now and in the future according to the distribution spec.

Comment on lines 138 to 153
"definition": {
"packagingDefinition": {
"type": "PrebuiltDockerImage"
"dockerImage": "hasura/ndc-clickhouse:v0.1.0"
},
"supportedEnvironmentVariables": [
{
"name": "username",
"description": "user name to access click house DB"
}
],
"cliPlugin": {
// fill in the blanks
},
...
}
Copy link
Member

Choose a reason for hiding this comment

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

since this information is being already submitted via the package definition archive, should we omit it to avoid duplication? Right now, the two pieces of information that gets stored in the hub registry DB is packagingDefinition.type and packagingDefinition.dockerImage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it might be worth including in the hub since there's no way to browse or search for that information inside a tarball. This way it can be indexed and displayed without having to download the distribution first. IMO it's worth the extra storage.

Copy link
Member

@scriptnull scriptnull Feb 26, 2024

Choose a reason for hiding this comment

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

Maybe we store only those two pieces of information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd store all of it in a JSONB or something personally since you never know what information someone will want. What's the motivation for storing less? Space usage shouldn't be much of an issue.

Copy link
Member

Choose a reason for hiding this comment

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

ah cool, sorry! Didn't notice your above comment. (GitHub failed in delivering that notification to my browser 😄)

Copy link
Member

Choose a reason for hiding this comment

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

@imperfect-fourth If we store this definition in our DB as jsonb, will it help skip the tar unpacking step for PreBuildDockerImage type of connectors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory!

Copy link
Member

Choose a reason for hiding this comment

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

if we store this definition in our DB as jsonb, will it help skip the tar unpacking step for PreBuildDockerImage type of connectors?

we are not doing this currently either since we have type and image columns in the connector table

@sordina sordina self-assigned this Mar 1, 2024
Comment on lines 137 to 152
// Imported directly from the package definition archive - See: https://github.com/hasura/ndc-hub/pull/89
"definition": {
"packagingDefinition": {
"type": "PrebuiltDockerImage"
"dockerImage": "hasura/ndc-clickhouse:v0.1.0"
},
"supportedEnvironmentVariables": [
{
"name": "username",
"description": "user name to access click house DB"
}
],
"cliPlugin": {
// fill in the blanks
},
...
Copy link
Member

Choose a reason for hiding this comment

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

let us remove this as we will be directly uploading from the package.tgz to db if needed

Copy link
Member

@scriptnull scriptnull left a comment

Choose a reason for hiding this comment

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

@sordina sordina merged commit 7247041 into main Mar 12, 2024
4 checks passed
@sordina sordina deleted the lyndon/distribution-rfc-milestone-1 branch March 12, 2024 05:17
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