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

Add a plugin for /pony #10422

Merged
merged 1 commit into from Dec 14, 2018

Conversation

Projects
None yet
7 participants
@Katharine
Copy link
Member

Katharine commented Dec 13, 2018

This critical PR adds a /pony command, and thereby closes #10415.

It is based very heavily on the code and tests for the dog and cat plugins (which, in turn, appear to be very heavily based on each other).

Usage is either of the following:

  • /pony for a completely random pony, or
  • /pony Pony Name, for a pony by that name.

There is also some non-name tag support, such as gif to get only animated ponies. They can be combined with commas:

  • /pony gif for a random animated pony.
  • /pony gif, Pony Name for an animated pony by that name.

Random ponies are collected from https://theponyapi.com.

@Katharine

This comment has been minimized.

Copy link
Member

Katharine commented Dec 13, 2018

Grr, hit the wrong keys and submitted a half-typed PR.

/cc @fejta

@k8s-ci-robot k8s-ci-robot requested a review from fejta Dec 13, 2018

}

const (
ponyURL = realHerd("https://theponyapi.com/pony.json")

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Dec 13, 2018

Contributor

Is this the API with which we will need to be careful? Does it expose user-provided content? Do we run the risk of posting inappropriate images here?

This comment has been minimized.

@Katharine

Katharine Dec 13, 2018

Member

All the images returned by this API (which is not the same one discussed previously, though that is its root data source) have been manually reviewed for appropriateness. It should be at least as safe as /meow

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Dec 13, 2018

Contributor

@ixdy didn't we just have an oopsie with the cat plugin?

This comment has been minimized.

@ixdy

ixdy Dec 14, 2018

Member

@stevekuznetsov yes, there was apparently a bug in which images which were rejected were still available via a direct request. it's supposedly fixed now; all cat images are supposed to be reviewed first as well.

@liztio

This comment has been minimized.

Copy link
Member

liztio commented Dec 14, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 14, 2018

LGTM label has been added.

Git tree hash: 966c18de16dc267109de4be0c6bcd7143b560891

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 14, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Katharine, liztio

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit ee1f7ea into kubernetes:master Dec 14, 2018

12 checks passed

cla/linuxfoundation Katharine authorized
Details
pull-test-infra-bazel Job succeeded.
Details
pull-test-infra-gubernator Skipped
pull-test-infra-lint Job succeeded.
Details
pull-test-infra-verify-bazel Job succeeded.
Details
pull-test-infra-verify-codegen Job succeeded.
Details
pull-test-infra-verify-config Job succeeded.
Details
pull-test-infra-verify-deps Skipped
pull-test-infra-verify-gofmt Job succeeded.
Details
pull-test-infra-verify-govet Job succeeded.
Details
pull-test-infra-verify-labels Skipped
tide In merge pool.
Details
@fejta

This comment has been minimized.

Copy link
Contributor

fejta commented Dec 14, 2018

YES!

@fejta

This comment has been minimized.

Copy link
Contributor

fejta commented Dec 14, 2018

We need to deploy a new version of prow before we can enable this

@ixdy

This comment has been minimized.

Copy link
Member

ixdy commented Dec 14, 2018

@fejta conveniently #10392

@fejta

This comment has been minimized.

Copy link
Contributor

fejta commented Dec 14, 2018

Yeah except it fails tests somehow!

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Dec 15, 2018

I enthusiastically approve this PR :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment