Skip to content

Commit

Permalink
Update cat plugin to use new JSON cat api
Browse files Browse the repository at this point in the history
  • Loading branch information
@ixdy
ixdy committed Aug 24, 2018
1 parent 4876b6c commit ff62132
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 117 deletions.
68 changes: 26 additions & 42 deletions prow/plugins/cat/cat.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ limitations under the License.
package cat

import (
"encoding/xml"
"encoding/json"
"errors"
"fmt"
"io/ioutil"
Expand All @@ -37,9 +37,9 @@ import (
)

var (
match = regexp.MustCompile(`(?mi)^/meow( .+)?\s*$`)
match = regexp.MustCompile(`(?mi)^/(meow)\s*$`)
meow = &realClowder{
url: "http://thecatapi.com/api/images/get?format=xml&results_per_page=1",
url: "https://api.thecatapi.com/api/images/get?format=json",
}
)

Expand All @@ -61,7 +61,7 @@ func helpProvider(config *plugins.Configuration, enabledRepos []string) (*plugin
Description: "Add a cat image to the issue",
Featured: false,
WhoCanUse: "Anyone",
Examples: []string{"/meow", "/meow caturday"},
Examples: []string{"/meow", "/meow"},
})
return pluginHelp, nil
}
Expand All @@ -71,7 +71,7 @@ type githubClient interface {
}

type clowder interface {
readCat(string) (string, error)
readCat() (string, error)
}

type realClowder struct {
Expand Down Expand Up @@ -103,44 +103,34 @@ func (c *realClowder) setKey(keyPath string, log *logrus.Entry) {
}

type catResult struct {
Source string `xml:"data>images>image>source_url"`
Image string `xml:"data>images>image>url"`
Id string `json:"id"`
URL string `json:"url"`
}

func (cr catResult) Format() (string, error) {
if cr.Source == "" {
return "", errors.New("empty source_url")
}
if cr.Image == "" {
if cr.URL == "" {
return "", errors.New("empty image url")
}
src, err := url.Parse(cr.Source)
if err != nil {
return "", fmt.Errorf("invalid source_url %s: %v", cr.Source, err)
}
img, err := url.Parse(cr.Image)
url, err := url.Parse(cr.URL)
if err != nil {
return "", fmt.Errorf("invalid image url %s: %v", cr.Image, err)
return "", fmt.Errorf("invalid image url %s: %v", cr.URL, err)
}

return fmt.Sprintf("[![cat image](%s)](%s)", img, src), nil
return fmt.Sprintf("[![cat image](%s)](%s)", url, url), nil
}

func (r *realClowder) Url(category string) string {
func (r *realClowder) Url() string {
r.lock.RLock()
defer r.lock.RUnlock()
uri := string(r.url)
if category != "" {
uri += "&category=" + url.QueryEscape(category)
}
if r.key != "" {
uri += "&api_key=" + url.QueryEscape(r.key)
}
return uri
}

func (r *realClowder) readCat(category string) (string, error) {
uri := r.Url(category)
func (r *realClowder) readCat() (string, error) {
uri := r.Url()
resp, err := http.Get(uri)
if err != nil {
return "", fmt.Errorf("could not read cat from %s: %v", uri, err)
Expand All @@ -149,19 +139,23 @@ func (r *realClowder) readCat(category string) (string, error) {
if sc := resp.StatusCode; sc > 299 || sc < 200 {
return "", fmt.Errorf("failing %d response from %s", sc, uri)
}
var a catResult
if err = xml.NewDecoder(resp.Body).Decode(&a); err != nil {
cats := make([]catResult, 0)
if err = json.NewDecoder(resp.Body).Decode(&cats); err != nil {
return "", err
}
if a.Image == "" {
if len(cats) < 1 {
return "", fmt.Errorf("no cats in response from %s", uri)
}
var a catResult = cats[0]
if a.URL == "" {
return "", fmt.Errorf("no image url in response from %s", uri)
}
// checking size, GitHub doesn't support big images
toobig, err := github.ImageTooBig(a.Image)
toobig, err := github.ImageTooBig(a.URL)
if err != nil {
return "", fmt.Errorf("could not validate image size %s: %v", a.Image, err)
return "", fmt.Errorf("could not validate image size %s: %v", a.URL, err)
} else if toobig {
return "", fmt.Errorf("longcat is too long: %s", a.Image)
return "", fmt.Errorf("longcat is too long: %s", a.URL)
}
return a.Format()
}
Expand Down Expand Up @@ -190,30 +184,20 @@ func handle(gc githubClient, log *logrus.Entry, e *github.GenericCommentEvent, c
// Now that we know this is a relevant event we can set the key.
setKey()

category := mat[1]
if len(category) > 1 {
category = category[1:]
}

org := e.Repo.Owner.Login
repo := e.Repo.Name
number := e.Number

for i := 0; i < 3; i++ {
resp, err := c.readCat(category)
resp, err := c.readCat()
if err != nil {
log.WithError(err).Error("Failed to get cat img")
continue
}
return gc.CreateComment(org, repo, number, plugins.FormatResponseRaw(e.Body, e.HTMLURL, e.User.Login, resp))
}

var msg string
if category != "" {
msg = "Bad category. Please see http://thecatapi.com/api/categories/list"
} else {
msg = "http://thecatapi.com appears to be down"
}
msg := "https://thecatapi.com appears to be down"
if err := gc.CreateComment(org, repo, number, plugins.FormatResponseRaw(e.Body, e.HTMLURL, e.User.Login, msg)); err != nil {
log.WithError(err).Error("Failed to leave comment")
}
Expand Down
96 changes: 21 additions & 75 deletions prow/plugins/cat/cat_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package cat

import (
"errors"
"flag"
"fmt"
"io"
Expand All @@ -39,10 +38,7 @@ var human = flag.Bool("human", false, "Enable to run additional manual tests")
var category = flag.String("category", "", "Request a particular category if set")
var path = flag.String("key-path", "", "Path to api key if set")

func (c fakeClowder) readCat(category string) (string, error) {
if category == "error" {
return "", errors.New(string(c))
}
func (c fakeClowder) readCat() (string, error) {
return fmt.Sprintf("![fake cat image](%s)", c), nil
}

Expand All @@ -54,7 +50,7 @@ func TestRealCat(t *testing.T) {
meow.setKey(*path, logrus.WithField("plugin", pluginName))
}

if cat, err := meow.readCat(*category); err != nil {
if cat, err := meow.readCat(); err != nil {
t.Errorf("Could not read cats from %#v: %v", meow, err)
} else {
fmt.Println(cat)
Expand All @@ -63,12 +59,10 @@ func TestRealCat(t *testing.T) {

func TestUrl(t *testing.T) {
cases := []struct {
name string
url string
category string
key string
require []string
deny []string
name string
url string
key string
require []string
}{
{
name: "only url",
Expand All @@ -79,21 +73,6 @@ func TestUrl(t *testing.T) {
url: "http://foo",
key: "blah",
require: []string{"api_key=blah"},
deny: []string{"category="},
},
{
name: "category",
url: "http://foo",
category: "bar",
require: []string{"category=bar"},
deny: []string{"api_key="},
},
{
name: "category and key",
url: "http://foo",
category: "this",
key: "that",
require: []string{"category=this", "api_key=that", "&"},
},
}

Expand All @@ -102,17 +81,12 @@ func TestUrl(t *testing.T) {
url: tc.url,
key: tc.key,
}
url := rc.Url(tc.category)
url := rc.Url()
for _, r := range tc.require {
if !strings.Contains(url, r) {
t.Errorf("%s: %s does not contain %s", tc.name, url, r)
}
}
for _, d := range tc.deny {
if strings.Contains(url, d) {
t.Errorf("%s: %s contained unexpected %s", tc.name, url, d)
}
}
}
}

Expand All @@ -121,45 +95,29 @@ func TestFormat(t *testing.T) {
basicURL := "http://example.com"
testcases := []struct {
name string
src string
img string
url string
err bool
}{
{
name: "basically works",
src: basicURL,
img: basicURL,
url: basicURL,
err: false,
},
{
name: "empty source",
src: "",
img: basicURL,
err: true,
},
{
name: "empty image",
src: basicURL,
img: "",
err: true,
},
{
name: "bad source",
src: "http://this is not a url",
img: basicURL,
url: "",
err: true,
},
{
name: "bad image",
src: basicURL,
img: "http://still a bad url",
url: "http://still a bad url",
err: true,
},
}
for _, tc := range testcases {
ret, err := catResult{
Source: tc.src,
Image: tc.img,
Id: tc.name,
URL: tc.url,
}.Format()
switch {
case tc.err:
Expand Down Expand Up @@ -196,8 +154,7 @@ func TestHttpResponse(t *testing.T) {
// create test cases for handling http responses
img := ts2.URL + "/cat.jpg"
bigimg := ts2.URL + "/bigcat.jpg"
src := "http://localhost?kind=source_url"
validResponse := fmt.Sprintf(`<response><data><images><image><url>%s</url><source_url>%s</source_url></image></images></data></response>`, img, src)
validResponse := fmt.Sprintf(`[{"id":"valid","url":"%s"}]`, img)
var testcases = []struct {
name string
path string
Expand All @@ -208,13 +165,13 @@ func TestHttpResponse(t *testing.T) {
{
name: "valid",
path: "/valid",
response: fmt.Sprintf(`<response><data><images><image><url>%s</url><source_url>%s</source_url></image></images></data></response>`, img, src),
response: validResponse,
valid: true,
},
{
name: "image too big",
path: "/too-big",
response: fmt.Sprintf(`<response><data><images><image><url>%s</url><source_url>%s</source_url></image></images></data></response>`, bigimg, src),
response: fmt.Sprintf(`[{"id":"toobig","url":"%s"}]`, bigimg),
},
{
name: "return-406",
Expand All @@ -234,9 +191,9 @@ Available variants:
</body></html>`,
},
{
name: "no-image-in-xml",
path: "/no-image-in-xml",
response: "<random><xml/></random>",
name: "no-cats-in-json",
path: "/no-cats-in-json",
response: "[]",
},
}

Expand Down Expand Up @@ -270,7 +227,7 @@ Available variants:
// run test for each case
for _, testcase := range testcases {
fakemeow := &realClowder{url: ts.URL + testcase.path}
cat, err := fakemeow.readCat(*category)
cat, err := fakemeow.readCat()
if testcase.valid && err != nil {
t.Errorf("For case %s, didn't expect error: %v", testcase.name, err)
} else if !testcase.valid && err == nil {
Expand Down Expand Up @@ -299,10 +256,7 @@ Available variants:
}
if c := fc.IssueComments[5][0]; !strings.Contains(c.Body, img) {
t.Errorf("missing image url: %s from comment: %v", img, c)
} else if !strings.Contains(c.Body, src) {
t.Errorf("missing source url: %s from comment: %v", src, c)
}

}

// Small, unit tests
Expand Down Expand Up @@ -354,17 +308,9 @@ func TestCats(t *testing.T) {
state: "open",
action: github.GenericCommentActionCreated,
body: "/meow clothes",
shouldComment: true,
shouldComment: false,
shouldError: false,
},
{
name: "bad cat",
state: "open",
action: github.GenericCommentActionCreated,
body: "/meow error",
shouldComment: true,
shouldError: true,
},
}
for _, tc := range testcases {
fc := &fakegithub.FakeClient{
Expand Down

0 comments on commit ff62132

Please sign in to comment.