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

feat: add siren proto with validation #27

Merged
merged 25 commits into from
Nov 16, 2021
Merged

feat: add siren proto with validation #27

merged 25 commits into from
Nov 16, 2021

Conversation

pyadav
Copy link
Member

@pyadav pyadav commented Aug 27, 2021

Added all proto required to generate GRPC code for siren

string updated_at = 8;
}

message Annotations {
Copy link
Member

Choose a reason for hiding this comment

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

Lets use singular word for object/message name?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, each key inside this message is an "Annotation". So that's why it makes more sense to call it plural.

Copy link
Member Author

@pyadav pyadav Aug 30, 2021

Choose a reason for hiding this comment

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

@rahmatrhd Changed Alerts to Alert

odpf/siren/siren.proto Outdated Show resolved Hide resolved
Copy link
Member

@rahmatrhd rahmatrhd left a comment

Choose a reason for hiding this comment

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

LGTM

odpf/siren/siren.proto Outdated Show resolved Hide resolved
odpf/siren/siren.proto Outdated Show resolved Hide resolved
@pyadav pyadav changed the title feat: add siren proto feat: add siren proto with validation Sep 6, 2021
@pyadav pyadav force-pushed the proto/siren branch 2 times, most recently from 4c64b1e to 5723a55 Compare September 6, 2021 10:10
string namespace = 4 [(validate.rules).string.pattern = "^[A-Za-z0-9_-]+$"];
string group_name = 5 [(validate.rules).string.pattern = "^[A-Za-z0-9_-]+$"];
string template = 6 [(validate.rules).string.pattern = "^[A-Za-z0-9_-]+$"];
string status = 7 [(validate.rules).string = {in: ["enabled", "disabled"]}];
Copy link
Member

Choose a reason for hiding this comment

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

Can we can convert status field to enum?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not doing this because downstream service accepts strings in order to implement this we need to put unnecessary type conversion in the middle which I would like to avoid.

string entity = 2 [(validate.rules).string.pattern = "^[A-Za-z0-9_-]+$"];
string message = 3;
string receiver_name = 4 [(validate.rules).string.pattern = "^[A-Za-z0-9_-]+$"];
string receiver_type = 5 [(validate.rules).string = {in: ["channel", "user"]}];;
Copy link
Member

Choose a reason for hiding this comment

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

here also, can we make reciever_type as enum? wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not doing this too, First, we will need to find a solution for Enum proto to pass case insensitive string.

@pyadav pyadav force-pushed the proto/siren branch 2 times, most recently from e5f53ab to 968ce0e Compare October 6, 2021 08:54
@pyadav pyadav force-pushed the proto/siren branch 3 times, most recently from 33ccc3e to 75a4cf4 Compare October 20, 2021 11:46
@ravisuhag ravisuhag merged commit 7f15c34 into main Nov 16, 2021
@ravisuhag ravisuhag deleted the proto/siren branch November 16, 2021 15:29
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.

6 participants