Added a -label-on-resolved flag. #38

cjyar · 2020-04-29T23:26:18Z

This addresses #37.

This change is

cjyar · 2020-04-29T23:28:33Z

issues/handler_test.go

@@ -47,7 +47,7 @@ func TestListHandler_ServeHTTP(t *testing.T) {
 			expectedStatus: http.StatusOK,
 			listClient: &fakeClient{
 				issues: []*github.Issue{
-					&github.Issue{


I know it seems unrelated, but this cleans up a lint warning I was getting from go1.14.

cjyar · 2020-04-29T23:28:56Z

issues/local/local.go

@@ -13,7 +13,7 @@
 // limitations under the License.
 //////////////////////////////////////////////////////////////////////////////

-// Package memory provides in memory operations on GitHub issues.
+// Package local provides in memory operations on GitHub issues.


Just fixing another lint warning here.

stephen-soltesz

Reviewable status: 0 of 1 LGTMs obtained

Dockerfile, line 1 at r1 (raw file):

FROM golang:1.14 as builder

It looks like the travis build is failing. I suspect .travis.yml needs a version update also.

Dockerfile, line 3 at r1 (raw file):

FROM golang:1.14 as builder

WORKDIR /src

I understand the need for go mod files. Why is a directory outside of /go/src necessary? This doesn't feel right. The build is working against/around the conventions for the goland base image. I think we can work with them.

alerts/handler.go, line 180 at r1 (raw file):

			}
		} else {
			err = rh.Client.LabelIssue(foundIssue, rh.ResolvedLabel, false)

Below there's a check for rh.ResolvedLabel != "" -- does an empty label behave differently in these two cases?

Simpler call sites like this are preferable. Would it be possible to move the rh.ResolvedLabel check to the LabelIssues function (and just return as a no/op with out error if the value is empty)?

issues/issues.go, line 137 at r1 (raw file):

// no error is returned if trying to add a label that's already present or
// remove one that's absent.
func (c *Client) LabelIssue(issue *github.Issue, label string, add bool) error {

Please ensure test coverage is preserved.

issues/local/local.go, line 16 at r1 (raw file):

Previously, cjyar (Chris Jones) wrote…

Just fixing another lint warning here.

Thank you!

stephen-soltesz

Thank you for adding the additional support for go1.14!

Reviewable status: 0 of 1 LGTMs obtained

cjyar

It sort of fell out of adding go.mod. That file wants to know the Go version, and my IDE filled in go1.14. Rather than downgrade it, I upgraded the other references to Go.

Reviewable status: 0 of 1 LGTMs obtained

Dockerfile, line 1 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

It looks like the travis build is failing. I suspect .travis.yml needs a version update also.

Will do.

Dockerfile, line 3 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

I understand the need for go mod files. Why is a directory outside of /go/src necessary? This doesn't feel right. The build is working against/around the conventions for the goland base image. I think we can work with them.

I thought if the source tree was under $GOROOT, it deactivates go modules. But you're right; I restored the previous path and it works.

go.mod, line 6 at r1 (raw file):

require (
	github.com/google/go-github v17.0.0+incompatible

This is now a pretty old version of go-github. It could use an update, but I think that's beyond the scope of this PR.

alerts/handler.go, line 180 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Below there's a check for rh.ResolvedLabel != "" -- does an empty label behave differently in these two cases?

Simpler call sites like this are preferable. Would it be possible to move the rh.ResolvedLabel check to the LabelIssues function (and just return as a no/op with out error if the value is empty)?

Thanks for catching that. I'll move that check to LabelIssue, as you suggest.

issues/issues.go, line 137 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Please ensure test coverage is preserved.

Will do.

coveralls · 2020-04-30T15:43:38Z

Pull Request Test Coverage Report for Build 126

66 of 66 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 122:	0.0%
Covered Lines:	368
Relevant Lines:	368

💛 - Coveralls

cjyar

Reviewable status: 0 of 1 LGTMs obtained

Dockerfile, line 1 at r1 (raw file):

Previously, cjyar (Chris Jones) wrote…

Will do.

Done.

Dockerfile, line 3 at r1 (raw file):

Previously, cjyar (Chris Jones) wrote…

I thought if the source tree was under $GOROOT, it deactivates go modules. But you're right; I restored the previous path and it works.

Done.

issues/issues.go, line 137 at r1 (raw file):

Previously, cjyar (Chris Jones) wrote…

Will do.

Done.

cjyar · 2020-04-30T17:41:16Z

@stephen-soltesz Ready for re-review.

stephen-soltesz

with two small nits. With those changes, I'll merge the result.

Thank you for your help!

Reviewed 5 of 11 files at r1, 5 of 5 files at r2, 3 of 3 files at r3.
Reviewable status: complete! 1 of 1 LGTMs obtained

Dockerfile, line 3 at r1 (raw file):

Previously, cjyar (Chris Jones) wrote…

Done.

Thank you!

go.mod, line 6 at r1 (raw file):

Previously, cjyar (Chris Jones) wrote…

This is now a pretty old version of go-github. It could use an update, but I think that's beyond the scope of this PR.

Agreed. I'll bet the alertmanager version is pretty old too. I've added an issue for that #39

issues/issues_test.go, line 285 at r3 (raw file):

					return
				}
				// r.ParseForm()

nit: please remove unused comment.

issues/issues_test.go, line 287 at r3 (raw file):

				// r.ParseForm()
				response := fmt.Sprintf(`[{}]`)
				w.Write([]byte(response))

nit: Please inline the value [{}] in the Write without the intermediate variable.

cjyar

One final thought: You might want to verify that the right metrics are being updated. I didn't spend a lot of time thinking about that.

Reviewable status: complete! 1 of 1 LGTMs obtained

issues/issues_test.go, line 285 at r3 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

nit: please remove unused comment.

Done. It was copypasta, and I removed the unused comment I copied it from also.

issues/issues_test.go, line 287 at r3 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

nit: Please inline the value [{}] in the Write without the intermediate variable.

Done.

stephen-soltesz

Reviewed 1 of 1 files at r4.
Reviewable status: complete! 1 of 1 LGTMs obtained

cjyar · 2020-04-30T18:13:24Z

Now I'm embarrassed. It looks like the GitHub API for deleting a label from an issue isn't idempotent, so this code doesn't actually work. :(

I was deploying it in our test instance while you were merging; too slow on this end.

I'll work on a fix.

stephen-soltesz · 2020-04-30T18:14:41Z

Oh no! Thanks for noticing quickly.

Added a -label-on-resolved flag.

39ed1a1

cjyar commented Apr 29, 2020

View reviewed changes

stephen-soltesz suggested changes Apr 30, 2020

View reviewed changes

stephen-soltesz reviewed Apr 30, 2020

View reviewed changes

Feedback from code review.

af0744e

cjyar commented Apr 30, 2020

View reviewed changes

More/better tests for more coverage.

d442f60

cjyar commented Apr 30, 2020

View reviewed changes

stephen-soltesz suggested changes Apr 30, 2020

View reviewed changes

Code review feedback.

9b407a1

cjyar commented Apr 30, 2020

View reviewed changes

stephen-soltesz approved these changes Apr 30, 2020

View reviewed changes

stephen-soltesz merged commit 041f88b into m-lab:master Apr 30, 2020

cjyar deleted the 37-relabel-resolved branch April 30, 2020 18:13

cjyar mentioned this pull request Apr 30, 2020

Make issues.Client.LabelIssue() idempotent for delete. #40

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a -label-on-resolved flag. #38

Added a -label-on-resolved flag. #38

cjyar commented Apr 29, 2020 •

edited by gfr10598

Loading

cjyar Apr 29, 2020

cjyar Apr 29, 2020

stephen-soltesz left a comment

stephen-soltesz left a comment

cjyar left a comment

coveralls commented Apr 30, 2020 •

edited

Loading

cjyar left a comment

cjyar commented Apr 30, 2020

stephen-soltesz left a comment

cjyar left a comment

stephen-soltesz left a comment

cjyar commented Apr 30, 2020

stephen-soltesz commented Apr 30, 2020

Added a -label-on-resolved flag. #38

Added a -label-on-resolved flag. #38

Conversation

cjyar commented Apr 29, 2020 • edited by gfr10598 Loading

cjyar Apr 29, 2020

Choose a reason for hiding this comment

cjyar Apr 29, 2020

Choose a reason for hiding this comment

stephen-soltesz left a comment

Choose a reason for hiding this comment

stephen-soltesz left a comment

Choose a reason for hiding this comment

cjyar left a comment

Choose a reason for hiding this comment

coveralls commented Apr 30, 2020 • edited Loading

Pull Request Test Coverage Report for Build 126

💛 - Coveralls

cjyar left a comment

Choose a reason for hiding this comment

cjyar commented Apr 30, 2020

stephen-soltesz left a comment

Choose a reason for hiding this comment

cjyar left a comment

Choose a reason for hiding this comment

stephen-soltesz left a comment

Choose a reason for hiding this comment

cjyar commented Apr 30, 2020

stephen-soltesz commented Apr 30, 2020

cjyar commented Apr 29, 2020 •

edited by gfr10598

Loading

coveralls commented Apr 30, 2020 •

edited

Loading