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

Draft: Updated Webhook types #1674

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

Conversation

smit-modi
Copy link
Contributor

@smit-modi smit-modi commented Mar 15, 2023

Added missing fields in DeploymentEvent.

Since GitLab doesn't send time data in RFC3339 format, currently, it is being unmarshalled as a string, I have added customTime as https://github.com/go-playground/webhooks/blob/master/gitlab/payload.go#L12 is doing.

For now only updated DeploymentEvent, planning to update other events as well soon.

@smit-modi smit-modi changed the title Updated DeploymentEvent Draft: Updated DeploymentEvent Mar 15, 2023
@smit-modi
Copy link
Contributor Author

@svanharmelen What are your thoughts on changing the fields with time data, from string to customTime type?

@svanharmelen
Copy link
Member

svanharmelen commented Mar 16, 2023

Hmm... We already have ISOTime as a custom time value, I'm not sure if I want to add more custom time objects.

Yet I do like your idea to try and parse multiple different formats, but that will only help parsing from the string to time.Time, not the other way around (for queries that take a time).

It's indeed a shame the GitLab API doesn't use a consistent time format, yet until now we seemed to be able to get away with just ISOTime and time.Time.

Guess I want to think about it a little and have a closer look when back behind a desk again (I'm away for a few days).

@klmitch
Copy link
Contributor

klmitch commented Mar 21, 2023

I thought ya'll had an alternative to time.Time that you were using, but I only saw time.Time in these structures. The name customTime is actually coming from other code that we're currently using. Is this something that we could consider adding to the ISOTime? Or could we perhaps add some additional functionality to ISOTime or customTime to track what format was read, for re-marshaling later. (Though we'd have to have some sort of functionality to map it to 3339, as that's what our existing code is intended to do...)

@smit-modi smit-modi changed the title Draft: Updated DeploymentEvent Draft: Updated Webhook types Mar 31, 2023
event_webhook_types.go Show resolved Hide resolved
HumanTotalTimeSpent string `json:"human_total_time_spent"` // TODO Check type
HumanTimeEstimate string `json:"human_time_estimate"` // TODO Check type
HumanTimeChange string `json:"human_time_change"` // TODO Check type
Weight string `json:"weight"` // TODO Check type
Copy link
Member

Choose a reason for hiding this comment

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

So did you check/confirm this? Can you share your findings? Same question goes for all places where you removed similar comments.

event_webhook_types.go Outdated Show resolved Hide resolved
@@ -672,15 +671,15 @@ type MergeEvent struct {
Repository *Repository `json:"repository"`
Labels []*EventLabel `json:"labels"`
Changes struct {
Description struct { // TODO GitLab doc doesn't mention this field
Copy link
Member

Choose a reason for hiding this comment

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

Same question, were you able to confirm this? Can you share where/how?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am keeping that field, even though the doc doesn't mention it, it does send Description change.

@svanharmelen
Copy link
Member

OK, I just noticed I commented on a specific commit instead of the whole PR and that the PR is still labelled as Draft 😊 So you should probably take my comments with a grain of salt...

I'll check again when the Draft label is removed and please also note that you can actually create a draft PR instead of just adding the text Draft as a label...

@smit-modi
Copy link
Contributor Author

I thought ya'll had an alternative to time.Time that you were using, but I only saw time.Time in these structures. The name customTime is actually coming from other code that we're currently using. Is this something that we could consider adding to the ISOTime? Or could we perhaps add some additional functionality to ISOTime or customTime to track what format was read, for re-marshaling later. (Though we'd have to have some sort of functionality to map it to 3339, as that's what our existing code is intended to do...)

@svanharmelen waiting for your response on this.

@GaikwadPratik
Copy link
Contributor

GaikwadPratik commented Apr 23, 2023

@smit-modi,

For your reference,
https://gitlab.com/gitlab-org/gitlab/-/issues/394785
https://gitlab.com/gitlab-org/gitlab/-/issues/19567

So we will need more than one formats, including RFC3339 to be handled, as I don't see any of the above ticket being resolved sooner or at all. They kept the same format as webhooks in graphql response for some reason when using time.RFC3339 is closest to being perfect format when used in UTC.

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.

4 participants