-
Notifications
You must be signed in to change notification settings - Fork 4
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
chore: move protos to canonical location #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest seems all good, let's settle a decision on the package name and I'll merge this.
proto/buf.yaml
Outdated
@@ -0,0 +1,10 @@ | |||
version: v1 | |||
name: buf.build/streamingfast/substreams-sink-database |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if it's the good package name substreams-sink-database
, I personally like the fact that we have -change
suffix, it gives a little help to get that it's about the changes and not sinking to any database (helps just a bit).
Also, do you think we should align GitHub repo name and buf protobuf package name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abourget Might also have some opinions here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to have all repos that contain sinks start with substreams-sink-
. I don't have a strong opinion about substreams-sink-database-change
vs. substreams-sink-database
. You are probably right that the suffix should include -change
for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok then I'll rename the project to substreams-sink-database-change
and then we align the buf
to be also substreams-sink-database-change
, good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to rename the rust crate too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would align everything indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will be a breaking change probably, I need to check what we put as the Rust crate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aw man, the proto package is named sf.substreams.sink.database.v1
. Would we rename that too? Seems like we are opening a can of worms here :-P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, is it database_change
or database_changes
? The message we are emitting is pluralized ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha :) So .... in all that, we have also https://github.com/streamingfast/substreams-entity-change which is more "tailored" made to graph-node
but which also have normalization issues.
With all those changes in those 2 packages, maybe we should actually do streamingfast/substreams-sink-sql#14. This would create a new version that would be incompatible anyway.
For which we could normalize fully and also be built to be use by graph-node
.
I've bring in most of this PR, the only thing I left for now is the rename of the Rust crate which is still I'll check to publish a new name when we will bridge entity-change and database-change together. |
... and configure buf.