-
Notifications
You must be signed in to change notification settings - Fork 6
Pull request for issue #40 #41
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
Conversation
Similar to google.api.resource, this commit introduces a new proto annotation for AEP resources. This allows for the extension of new metadata as needed. The annotation includes the following fields: - plural - singular - type - patterns - parents
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. New to Jules? Learn more at jules.google/docs. |
toumorokoshi
left a comment
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.
close! a few changes please.
proto/aep-api/aep/api/resource.proto
Outdated
| string singular = 4; | ||
|
|
||
| // The list of parent resource types. | ||
| repeated string parents = 5; |
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.
this should be "parent".
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.
Done. I have renamed parents to parent and updated the comment accordingly.
proto/aep-api/aep/api/resource.proto
Outdated
|
|
||
| extend google.protobuf.MessageOptions { | ||
| // A descriptor for a resource type. | ||
| ResourceDescriptor resource = 1059; |
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.
this should use a field extension in the range 1264-1274 (whatever is currently unused in this repository). This is part of the extension reserved by the aeps: https://github.com/protocolbuffers/protobuf/blob/main/docs/options.md
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.
Done. I have updated the field extension to 1264, which is within the recommended range.
| @@ -0,0 +1,54 @@ | |||
| // Copyright 2024 aep-dev. All Rights Reserved. | |||
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.
please add missing options as specified in the failing github workflows.
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.
Done. I have added the missing java and cc options to the file, using another .proto file in the repository as a reference.
|
the linting has failed with buf: please fix these. |
|
I have generated the requested changes, but I was unable to publish them to the branch. You can publish the changes manually from the Jules UI. |
Similar to google.api.resource, this commit introduces a new proto annotation for AEP resources. This allows for the extension of new metadata as needed. The annotation includes the following fields: - plural - singular - type - patterns - parent
|
seems like jules needs a bit of help here, closing for now. |
Acknowledged. |
I've fixed the linting errors by adding the missing |
Fixes #40
PR created automatically by Jules for task 3549055623913917094