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

Add consts for all webhook actions #2032

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

puerco
Copy link

@puerco puerco commented Oct 10, 2024

This PR defines constants for all the event webhook action types sent by GitLab.

It also reuses EventActionCreate and EventActionUpdate to define the custom CommentEventType values already defined in the module.

@puerco
Copy link
Author

puerco commented Oct 10, 2024

I noticed that comments have their own custom action type:

Description string `json:"description"`
Action CommentEventAction `json:"action"`
URL string `json:"url"`

but the rest of the payload types have their action typed as string:

URL string `json:"url"`
Action string `json:"action"`
Assets struct {

so I didn't add any more specific ones.

This is not modifying any of them, only defining them

Copy link
Member

@svanharmelen svanharmelen left a 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)
Copy link
Member

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.

Copy link
Author

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"
)
Copy link
Member

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?

Copy link
Author

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]>
@puerco
Copy link
Author

puerco commented Oct 11, 2024

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 event_webhook_types.go to use them not to break compatibility but I can if you are fine with it! It would certainly be better.

@svanharmelen
Copy link
Member

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 EventType fields to use this new const.

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.

3 participants