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

[feat]: [CDS-75848]: Add new action type for github provider #270

Merged
merged 3 commits into from
Sep 11, 2023

Conversation

rathodmeetsatish
Copy link
Contributor

No description provided.

@CLAassistant
Copy link

CLAassistant commented Aug 29, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@tphoney tphoney left a comment

Choose a reason for hiding this comment

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

The code implementation looks good and it has unit tests as well.
My issue would be, do other SCM providers have this concept ?
The whole point of go-scm is to abstract away implementation details from each SCM to give a generic way of dealing with SCM systems as a whole.
If this is only a github only feature, it does not make sense to add this as a new actiontype.

So can you look to see if this concept of pr approval exists in the other supported SCM webhooks. eg gitlab / gogs / bitbucket

@bradrydzewski
Copy link
Member

bradrydzewski commented Aug 31, 2023

I agree with TP. When adding new functionality to this library it is standard practice to provide implementations for more than one provider to ensure we have the correct abstractions in place. I recommend adding GitLab or Bitbucket to this pull request.

EDIT

It looks like other providers don't have this action type. However, with that being said, we could emulate this event for GitLab by looking at the changes section of the webhook to see if the draft field changed from true to false.

Example GitLab webhook.

  "changes": {
    "draft": {
      "previous": true,
      "current": false
    },
    "title": {
      "previous": "Draft: Update README.md",
      "current": "Update README.md"
    },
    "updated_at": {
      "previous": "2023-08-31 18:03:37 UTC",
      "current": "2023-08-31 18:04:01 UTC"
    },
    "updated_by_id": {
      "previous": null,
      "current": 8881996
    }
  }

Example change to GitLab driver:

func convertPullRequestHook(src *pullRequestHook) *scm.PullRequestHook {
	action := scm.ActionSync
	switch src.ObjectAttributes.Action {
	case "open":
		action = scm.ActionOpen
	case "close":
		action = scm.ActionClose
	case "reopen":
		action = scm.ActionReopen
	case "merge":
		action = scm.ActionMerge
	case "update":
		action = scm.ActionSync
+		// emulate the ready_for_review event by inspecting
+		// the change in draft status from true to false
+		if src.Changes.Draft.Previous.Bool == true {
+			action = scm.ReadyForReview
+		}
	}
		Changes struct {
+			Draft struct {
+					Previous null.Bool `json:"previous"
+					Current  null.Bool `json:"current"
+			} `json:"draft"`
			Labels struct {
				Previous []interface{} `json:"previous"`
				Current  []struct {
					ID          int         `json:"id"`
					Title       string      `json:"title"`
					Color       string      `json:"color"`
					ProjectID   int         `json:"project_id"`
					CreatedAt   string      `json:"created_at"`
					UpdatedAt   string      `json:"updated_at"`
					Template    bool        `json:"template"`
					Description string      `json:"description"`
					Type        string      `json:"type"`
					GroupID     interface{} `json:"group_id"`
				} `json:"current"`
			} `json:"labels"`
		} `json:"changes"`

I would also rename to review_ready so that we use review_ as the prefix. This will ensure consistency if we ever add other review_ actions in the future, such as review_request_add or review_request_remove

// Action values.
const (
	ActionUnknown Action = iota
	ActionCreate
	ActionUpdate
	ActionDelete
	// issues
	ActionOpen
	ActionReopen
	ActionClose
	ActionLabel
	ActionUnlabel
	// pull requests
	ActionSync
	ActionMerge
-	ActionReadyForReview
+	ActionReviewReady
	// issue comment
	ActionEdit
	// release
	ActionPublish
	ActionUnpublish
	ActionPrerelease
	ActionRelease
)

@bradrydzewski bradrydzewski merged commit 7065a05 into drone:master Sep 11, 2023
1 check passed
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.

None yet

4 participants