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

Fix: Sync project/group resource access token webhook events to match current documentation #1930

Merged
merged 1 commit into from
May 14, 2024

Conversation

vntw
Copy link
Contributor

@vntw vntw commented May 3, 2024

Update: After raising this issue, this merge request updated the webhook payloads that we can expect.

This PR updates the payloads and structs to reflect that ✌️


I searched for other places in the code where a format like 2024-01-24 16:27:40 UTC occurs and found (among others) this which I used as a reference:

CreatedAt string `json:"created_at"`
FinishedAt string `json:"finished_at"`

Hope that this is the correct way! Let me know if it's not

@svanharmelen
Copy link
Member

Hmm... Yeah the inconsistencies around how time is used in the API is quite frustrating. In this case I guess it would make sense to first open an issue with GitLab to see if it's a bug there which they need to fix instead?

@vntw
Copy link
Contributor Author

vntw commented May 6, 2024

Good idea, I'll create an issue (if there hasn't been one already) and report back

e/ issue created

@vntw vntw changed the title Fix date format assumption for GroupResourceAccessTokenEvent.CreatedAt Fix: Sync project/group resource access token webhook events to match current documentation May 14, 2024
@vntw
Copy link
Contributor Author

vntw commented May 14, 2024

In this case I guess it would make sense to first open an issue with GitLab to see if it's a bug there which they need to fix instead?

Thanks again for the hint, turns out there was more to it 😃 I updated the PR with the necessary links and info.

I think this is ready now!

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.

It's a shame the used time formats are such a mess. We could introduce a UTCTime struct like the ISOTime struct to enable parsing it as a time.Time, but for now I'm not going to bother and it could be an annoying change to existing users of the API.

So thanks for your efforts 👍🏻

@svanharmelen svanharmelen merged commit 16ebc6f into xanzy:main May 14, 2024
3 checks passed
@vntw vntw deleted the fix-gat-date branch May 16, 2024 15:14
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.

None yet

2 participants