feat(#27): change status based on existing label #189

hemanik · 2018-06-12T04:40:46Z

The current WIP plugin looks for the PR title prefix to set the status. The scope of this enhanced to change PR status based on the existing label.

The new behavior handles the following cases as described below:

set the status to failure if PR either contains a wip label (irrespective to the title prefix) or it has a wip prefix in the title (in which case the label is automatically added).
set the status to success if the PR is updated by either removing the wip prefix from the title (in which case the label is also removed automatically) or by removing the wip label (in which case, if present, the wip title prefix is also remove automatically).

Fixes: #27

centos-ci · 2018-06-12T04:40:47Z

Can one of the admins verify this patch?

MatousJobanek

Thanks @hemanik for this PR! I'm looking forward to having it running.
My comments are inline.

MatousJobanek · 2018-06-12T13:41:25Z

docs/fragments/status/work-in-progress/failure/wip-failed.adoc

@@ -1,8 +1,8 @@
 ==== Failed [[wip-failed]]

-Your Pull Request has been rejected because the plugin detected that the PR title starts with one of the <<index#work-in-progress-config,"work in progress prefixes">>, so it seems that there is still an ongoing work.
+Your Pull Request has been rejected because the plugin detected that either the PR title starts with one of the <<index#work-in-progress-config,"work in progress prefixes">> or has the "work-in-progress" label, so it seems that there is still an ongoing work.


WDYT about this:
...starts with one of the "work in progress prefixes" or has the "work-in-progress" label (both of them are <<index#work-in-progress-config, configurable>>), so it seems that there is still an ongoing work.

MatousJobanek · 2018-06-12T13:45:28Z

docs/fragments/work-in-progress.adoc

+* If the PR title starts with any of the default prefixes such as `WIP`, `DO NOT MERGE`, `DON'T MERGE`, `WORK-IN-PROGRESS` with combinations of `( )`, `[ ]`, `:` or any of the user configured prefixes as described below (non-case sensitive) then the plugin marks its check as **Failure**, or as **Success** otherwise.
+* Or if the PR contains the default (`const:pkg/plugin/work-in-progress/configuration.go[name="DefaultLabel"]`) or any other user configured label then the plugin marks its check as **Failure**, or as **Success** otherwise.
+
+In addition to managing the status for the PR in progress, it also manages adding/removing the GitHub label (`const:pkg/plugin/work-in-progress/configuration.go[name="DefaultLabel"]` by default or configured otherwise) and updating the PR title in the event of updating the given PR by either removing the prefix title or removing the label. 


I was thinking if it would make sense to write something similar to what you wrote in the issue: #27 (comment)

MatousJobanek · 2018-06-12T13:46:38Z

pkg/github/client/client.go

@@ -170,7 +172,17 @@ func (c *client) CreateStatus(change scm.RepositoryChange, repoStatus *gogh.Repo
 	return err
 }

-func (c client) ListPullRequestLabels(change scm.RepositoryChange, prNumber int) ([]*gogh.Label, error) {
+func (c *client) EditPullRequest(change scm.RepositoryChange, prNumber int, pullRequest *gogh.PullRequest) error {


both the prNumber and all information in change are available in the pullRequest instance as well - no need to provide it separately

MatousJobanek · 2018-06-12T13:51:32Z

pkg/plugin/work-in-progress/event_handler.go

-	return gh.checkTitleAndSetStatus(log, event.PullRequest)
-}
+	switch *event.Action {
+	case "labeled":


make it a constant in the types.go file

MatousJobanek · 2018-06-12T14:00:24Z

pkg/plugin/work-in-progress/event_handler.go

+	return "", false
+}
+
+func (gh *GitHubWIPPRHandler) getRepositoryChange(pullRequest *gogh.PullRequest) scm.RepositoryChange {


This should be placed next to the scm.RepositoryChange struct as its constructor. And by convention, the name should start with "New"

there is already such a function: https://github.com/arquillian/ike-prow-plugins/blob/master/pkg/github/service/repository_change.go so please use this one

MatousJobanek · 2018-06-12T14:23:31Z

pkg/plugin/work-in-progress/event_handler.go

+				log.Warnf("failed to update PR title [%q]. cause: %s", *pullRequest, err)
+			}
+		}
+		return statusService.Success(ReadyForReviewMessage, ReadyForReviewDetailsPageName)


if the label was removed, but the edit of the PR failed should we change the status to success as well? then it looks a bit inconsistent - the PR contains the prefix, but the status is green...
maybe, instead of logging the error https://github.com/arquillian/ike-prow-plugins/pull/189/files#diff-11865c15f9f479c8ca04795d06b465acR162 we should return it

MatousJobanek · 2018-06-12T14:23:56Z

pkg/plugin/work-in-progress/event_handler.go

+func (gh *GitHubWIPPRHandler) getPullRequestLabels(log log.Logger, change scm.RepositoryChange, pullRequest *gogh.PullRequest) []*gogh.Label {
+	labels, err := gh.Client.ListPullRequestLabels(change, *pullRequest.Number)
+	if err != nil {
+		log.Warnf("failed to list labels on PR [%q]. cause: %s", *pullRequest, err)


log it as an error.
btw, I think that you should call err.Error() to get the message

MatousJobanek · 2018-06-12T14:29:46Z

pkg/plugin/work-in-progress/event_handler.go

+}
+
+// IsWorkInProgress checks if title is marked as Work In Progress
+func (gh *GitHubWIPPRHandler) IsWorkInProgress(title string, config PluginConfiguration) (string, bool) {


with the ability to use labels, we should rename the method for example to: HasWorkInProgressPrefix

And I would probably swap the return parameters - the first would be bool, then the prefix. The reason is that the main purpose of this method is checking if the title contains the prefix, the retrieved prefix is just a side effect.

MatousJobanek · 2018-06-12T14:36:08Z

pkg/plugin/work-in-progress/event_handler.go

+	for _, prefix := range prefixes {
+		pattern := `(?mi)^(\[|\()?` + prefix + `(\]|\))?(:| )+`
+		if match, _ := regexp.MatchString(pattern, title); match {
+			return prefix, true


what if the prefix contains also brackets? are they matched and then removed as well? the same applies also to the colon.

MatousJobanek · 2018-06-12T14:38:00Z

pkg/plugin/work-in-progress/event_handler.go

+	}
+}
+
+func (gh *GitHubWIPPRHandler) getStatusService(log log.Logger, change scm.RepositoryChange) scm.StatusService {


name it newStatusService

MatousJobanek

Thanks for the changes. I have a few more comments, but nothing significant...

MatousJobanek · 2018-06-14T08:58:45Z

docs/fragments/status/work-in-progress/success/wip-success.adoc

@@ -1,6 +1,6 @@
 ==== Success [[wip-success]]

-Your Pull Request has been approved as the plugin didn't detect any sign of work in progress (the PR title doesn't start with any of the <<index#work-in-progress-config,"work in progress prefixes">>).
+Your Pull Request has been approved as the plugin didn't detect any sign of work in progress (the PR title doesn't start with any of the <<index#work-in-progress-config,"work in progress prefixes">> or contain any "work-in-progress" label).


Please, make it same as for the failure page - the redirection to the configuration options

I see that you have updated the failure page. 39dc7fa
I wanted to say that you should update the success page to be same as the failure page was, not the other way around. I see that my previous comment is very confusing and also grammatically wrong, so I do not wonder that you misunderstood. Sorry for that :-)
Could you please change both of them one more time? Thanks :-)

MatousJobanek · 2018-06-14T09:18:42Z

pkg/plugin/work-in-progress/event_handler.go

+		if labelUpdated {
+			*pullRequest.Title = strings.TrimPrefix(*pullRequest.Title, prefix)
+			if err := gh.Client.EditPullRequest(pullRequest); err != nil {
+				return fmt.Errorf("failed to update PR title [%q]. cause: %s", *pullRequest, err)


Correct me if I'm wrong - to get the nice format of the error (that contains a message of the original error) it is necessary to call err.Error() on the original error - that retrieves the message of the root cause.

err.Error() is not required since the fmt package formats an error value by calling its Error() string method.

OK, I wasn't sure about that - thanks for the info :-)

MatousJobanek · 2018-06-14T09:28:10Z

pkg/plugin/work-in-progress/event_handler.go

 			}
+			return statusService.Success(ReadyForReviewMessage, ReadyForReviewDetailsPageName)
+		}
+		if err := gh.Client.AddPullRequestLabel(change, *pullRequest.Number, strings.Fields(configuration.Label)); err != nil {


why strings.Fields? the label should be only one and can also contain a white space - eg: work in progress

The following function used to add label expects a string array of labels. We only have one string label and that needs to be converted to a string array and hence the choice of string.Field().

The choice of converting the string to string array is done at this location instead inside the gh.Client.AddPullRequestLabel because I wanted to keep this method unmodified considering this could be used elsewhere where we could have to send multiple label values. (Example: #57)

// AddLabelsToIssue adds labels to an issue. // // GitHub API docs: https://developer.github.com/v3/issues/labels/#add-labels-to-an-issue func (s *IssuesService) AddLabelsToIssue(ctx context.Context, owner string, repo string, number int, labels []string) ([]*Label, *Response, error) {

Thanks for the explanation @MatousJobanek, updated it.

MatousJobanek · 2018-06-14T09:36:47Z

pkg/plugin/work-in-progress/event_handler_unit_test.go

@@ -20,7 +20,8 @@ var _ = Describe("Work-in-progress Plugin features", func() {

 		DescribeTable("should recognize PR as work-in-progress if title starts with configured or default prefix",
 			func(title string) {
-				Expect(handler.IsWorkInProgress(title, wip.PluginConfiguration{Prefix: []string{`On Hold`}, Combine: true})).To(BeTrue())
+				state, _ := handler.HasWorkInProgressPrefix(title, wip.PluginConfiguration{Prefix: []string{`On Hold`}, Combine: true})


could you please also verify (in all cases) the retrieved prefix - this way you verify that the find works correctly

MatousJobanek · 2018-06-14T09:41:20Z

pkg/github/client/client.go

@@ -28,6 +29,7 @@ type Client interface {
 	ListPullRequestLabels(change scm.RepositoryChange, prNumber int) ([]*gogh.Label, error)


is this used anywhere?

MatousJobanek · 2018-06-14T09:43:40Z

pkg/plugin/work-in-progress/event_handler_gh_test.go

+				Reply(200).
+				BodyString(`[{"id": 934813958,` +
+					`"url": "https://api.github.com/repos/bartoszmajsak/wfswarm-booster-pipeline-test/labels/work-in-progress",` +
+					`"name": "work-in-progress", "color": "ededed", "default": false}]`)


this mock (and all other mocks for the labels listing) is not necessary - the information is taken from the PR

MatousJobanek · 2018-06-14T09:48:18Z

pkg/plugin/work-in-progress/event_handler_gh_test.go

+
+			gock.New("https://api.github.com").
+				Patch("repos/bartoszmajsak/wfswarm-booster-pipeline-test/pulls/4").
+				Reply(200).


please add also a matcher checking that the title was correctly updated - removed the prefix including the flavours

… into status-update

…lugins into status-update

MatousJobanek

Except for the failure and success page it looks good.
Good job @hemanik

MatousJobanek

LGTM 👍 🚀 🥇

MatousJobanek · 2018-06-15T06:59:23Z

[test]

hemanik added 2 commits June 11, 2018 23:26

feat(arquillian#27): change status based on existing label.

def8b81

docs: updates work in progress plugin description.

8ab4647

alien-ike added the size/XXL label Jun 12, 2018

hemanik requested review from dipak-pawar and MatousJobanek June 12, 2018 04:41

hemanik added 2 commits June 12, 2018 16:38

fix: misplaced return statement for the status update.

c2ef54f

refactor: abstract common logic.

7edd2cc

MatousJobanek requested changes Jun 12, 2018

View reviewed changes

hemanik added 7 commits June 13, 2018 16:30

chore: make action values constant and update function signature.

b3c8e0e

chore: rename method and remove reduntant methods.

ded2527

fix: error handling and logging.

2f46812

fix: merge multiple methods into one.

e5e9c2b

fix: prefix trimming from PR.

c97bf93

docs: update work in progress plugin description.

443dba9

Merge branch 'master' into status-update

f0e2549

MatousJobanek requested changes Jun 14, 2018

View reviewed changes

hemanik added 6 commits June 14, 2018 19:04

test: verify prefix in title.

b390cc6

chore: remove unused gocks and methods.

ba2e84b

docs: add redirection to config for failure status.

39dc7fa

Merge branch 'master' of https://github.com/arquillian/ike-prow-plugins…

bc1568a

… into status-update

Merge branch 'status-update' of https://github.com/hemanik/ike-prow-p…

5a60daf

…lugins into status-update

chore: replace strings.Field() with string slice.

8bc4359

MatousJobanek previously approved these changes Jun 14, 2018

View reviewed changes

docs: update wip failed and success state descriptions.

119492a

hemanik dismissed MatousJobanek’s stale review via 119492a June 15, 2018 06:25

MatousJobanek approved these changes Jun 15, 2018

View reviewed changes

MatousJobanek merged commit 43a7ffa into arquillian:master Jun 15, 2018

MatousJobanek mentioned this pull request Jun 19, 2018

feat(#6): PR Title Verification (Semantic Messages) #169

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(#27): change status based on existing label #189

feat(#27): change status based on existing label #189

hemanik commented Jun 12, 2018

centos-ci commented Jun 12, 2018

MatousJobanek left a comment

MatousJobanek Jun 12, 2018

MatousJobanek Jun 12, 2018

MatousJobanek Jun 12, 2018

MatousJobanek Jun 12, 2018

MatousJobanek Jun 12, 2018

MatousJobanek Jun 12, 2018

MatousJobanek Jun 12, 2018

MatousJobanek Jun 12, 2018

MatousJobanek Jun 12, 2018

MatousJobanek Jun 12, 2018

MatousJobanek Jun 12, 2018

MatousJobanek Jun 12, 2018

MatousJobanek left a comment

MatousJobanek Jun 14, 2018

MatousJobanek Jun 14, 2018

MatousJobanek Jun 14, 2018

hemanik Jun 14, 2018 •

edited

Loading

MatousJobanek Jun 14, 2018

MatousJobanek Jun 14, 2018

hemanik Jun 14, 2018

hemanik Jun 14, 2018

MatousJobanek Jun 14, 2018

MatousJobanek Jun 14, 2018

MatousJobanek Jun 14, 2018

MatousJobanek Jun 14, 2018

MatousJobanek left a comment

MatousJobanek left a comment

MatousJobanek commented Jun 15, 2018

		@@ -28,6 +29,7 @@ type Client interface {
		ListPullRequestLabels(change scm.RepositoryChange, prNumber int) ([]*gogh.Label, error)

feat(#27): change status based on existing label #189

feat(#27): change status based on existing label #189

Conversation

hemanik commented Jun 12, 2018

centos-ci commented Jun 12, 2018

MatousJobanek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatousJobanek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hemanik Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatousJobanek left a comment

Choose a reason for hiding this comment

MatousJobanek left a comment

Choose a reason for hiding this comment

MatousJobanek commented Jun 15, 2018

hemanik Jun 14, 2018 •

edited

Loading