-
Notifications
You must be signed in to change notification settings - Fork 963
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
Add consts for all webhook actions #2032
base: main
Are you sure you want to change the base?
Conversation
I noticed that comments have their own custom action type: go-gitlab/event_webhook_types.go Lines 272 to 274 in b5e0812
but the rest of the payload types have their action typed as string: go-gitlab/event_webhook_types.go Lines 1050 to 1052 in b5e0812
so I didn't add any more specific ones. This is not modifying any of them, only defining them |
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.
Besides the two (minor) comments, what is the end goal with this? Just defining them without using them doesn't have much use, so are you going to add move commits to this PR to also use these?
types.go
Outdated
CommentEventActionCreate CommentEventAction = "create" | ||
CommentEventActionUpdate CommentEventAction = "update" | ||
CommentEventActionCreate CommentEventAction = CommentEventAction(EventActionCreate) | ||
CommentEventActionUpdate CommentEventAction = CommentEventAction(EventActionUpdate) |
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 prefer to not tie these together and revert this change.
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 done 👍
types.go
Outdated
EventActionUnapproval EventAction = "unapproval" | ||
EventActionUnapproved EventAction = "unapproved" | ||
EventActionUpdate EventAction = "update" | ||
) |
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.
Stuff (types and consts) in this file is alphabetically ordered, so can you move these to the ordered location?
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.
Moved 👍
This commit defines constants for all the event webhook action types sent by GitLab. It reuses them to define the custom CommentEventType Signed-off-by: Adolfo García Veytia (puerco) <[email protected]>
837b37a
to
ae4069d
Compare
My main goal as a user is to expose the values of the action from the library to applications. I didn't want to modify the payload types in |
Yeah while it might cause a few issues, I think it's the best what to actually guarantee the list will stay up-to-date over time. So I would say go ahead and update the |
This PR defines constants for all the event webhook action types sent by GitLab.
It also reuses
EventActionCreate
andEventActionUpdate
to define the custom CommentEventType values already defined in the module.