feat: Add project proto & generate #27
Conversation
palp
left a comment
palp
left a comment
There was a problem hiding this comment.
Some consideration needs to be given to the way the protoc binary is used, but it's not blocking this from being merged.
| set(NODE_BIN_DIRECTORY "${PROJECT_SOURCE_DIR}/node_modules/.bin") | ||
|
|
||
| set(javascript_exec "protoc") | ||
| set(javascript_exec "${PROJECT_SOURCE_DIR}/src/bin/protoc") |
There was a problem hiding this comment.
This could cause the docker build to fail in the future, as well as failing on other platforms; I think it should either use a variable to point to protoc directly so that can be overridden by the build, or we should copy the binary to this location during the docker build.
There was a problem hiding this comment.
The binary is actually already in that location, and the docker build works, other platforms could be an issue as it is a linux amd64 binary. This is just putting this back in a state that actually works, as without it some systems (like mine) have a system protoc version that is not able to be used for TS/JS generation.
I would prefer us to not be building the proto definitions manually most of the time anyhow and relying on Actions here to build them as it provides a more controlled environment.
There was a problem hiding this comment.
Yeah, I'm aware it'll work fine in most scenarios, which is why I'm fine with this being merged as is - just wanted to make a note.
wbrown
left a comment
wbrown
left a comment
There was a problem hiding this comment.
Some general ideas and feedback.
src/proto/project.proto
Outdated
| } | ||
|
|
||
| // Message for empty requests / response where no additional data is needed | ||
| message ProjectEmpty { |
There was a problem hiding this comment.
What I've found useful is to return the data structure being deleted, but with a deleted status to communicate that X was deleted.
There was a problem hiding this comment.
I don't have any issue with returning it.
src/proto/project.proto
Outdated
| } | ||
|
|
||
| message Project { | ||
| uint64 id = 1; // ID of the project |
There was a problem hiding this comment.
suggest an uuid v4 and translating that to a binary lookup on the db/backend side.
There was a problem hiding this comment.
If we use a UUID here, then we should be using across the board for these in my opinion no reason to translate UUID to ID.
- Change project id to UUIDv4 instead of uint64 - Change asset id to UUIDv4 instead of uint64 - Rename `owner` to `owner_id` for project to be more clear - Add undefined usage to `ProjectAssetUse` - Add deleted status to `ProjectStatus` - Update `Delete` rpc to return project - Add `uuid` and `size` to artifact - Add enumeration for asset usage to generation.proto to mirror that of the usage enum in project.proto, current files have some issues with importing / sharing proto definitions. - Add asset usage to `AssetParameters` - Regenerate files
|
@arsenetar I'm ok with making the |
|
Kudos, SonarCloud Quality Gate passed!
|
Initial version, may have slight addition needed to
AssetParametersto fully implement.Also fixed issue with no longer using the provided
protocfor js (systemprotocmay be a broken version).