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

Support fetching labels from pull requests in github,gitlab & gitea #67

Merged
merged 4 commits into from
Aug 19, 2020

Conversation

takirala
Copy link
Contributor

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jul 17, 2020

CLA assistant check
All committers have signed the CLA.

@bradrydzewski
Copy link
Member

this looks good. my only request would be to ensure that one of the sample pull request webhooks for GitHub has labels so that the conversion is covered by unit tests.

@bradrydzewski
Copy link
Member

bradrydzewski commented Aug 19, 2020

on second thought ... I wonder if we should return an array of Label structures instead of a string array. The downside to a string array is that if we ever decide we need to expose more details, it would require a breaking change. So perhaps it is just best to provide the full details from the onset ...

Label struct {
  Name  string
  Color string
}

@takirala
Copy link
Contributor Author

@bradrydzewski i have added

Label struct {
  Name  string
}

i did not add color field for this reason : gitlab does not provide it in merge api response. it is however "fetchable" by making another API call. also, since this change would be now backwards compatible we can always add a color field (or any other fields) in the future as needed.

Also updated the unit tests to validate the conversions. lmk what you think.

@bradrydzewski
Copy link
Member

bradrydzewski commented Aug 19, 2020

I plan on adding methods to create / find / list / delete labels so it would be safe to add the Color field in the structure at this time :) There are other examples of webhooks where fields are blank, for example this comment from another issue:

when processing an issue comment event from a pull request, we might need to issue another http request to get the pull request information which is not in the event payload.

The most important part is making sure the structure is correct and flexible enough to support all providers. It is ok that the fields may be blank for a subset of providers, as long as we aren't adding fields that are specific to a single provider.

@takirala
Copy link
Contributor Author

@bradrydzewski sounds good, updated the pr!

@bradrydzewski
Copy link
Member

thanks!

@bradrydzewski bradrydzewski merged commit 565d225 into drone:master Aug 19, 2020
@takirala takirala deleted the tga/add-labels-support branch August 20, 2020 02:34
jstrachan pushed a commit to jstrachan/go-scm that referenced this pull request Dec 22, 2020
feat: Record the create pull request in the fake driver.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants