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

Jump to bottom

slack-bot: handle interactions by filing issues #1264

Merged
merged 2 commits into from Oct 14, 2020
Merged

slack-bot: handle interactions by filing issues #1264

merged 2 commits into from Oct 14, 2020

Conversation

stevekuznetsov
Copy link
Contributor

With this patch, the test platform Slack integration now knows how to
respond to a number of shortcut actions from users, opening dialogs that
prompt them for information in order to open issues on the DPTP Jira
board. The following types of issues are supported:

  • bugs
  • consulting requests
  • feature requests
  • incident tracking cards

Input validation is achieved with custom validation on submissiosn as
well as built-in Slack modal features; users have interactive flows to
provide them feedback on the inputs and allow them to open bugs against
this tool if we run into unexpected issues.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2020
@stevekuznetsov
Copy link
Contributor Author 

$ make update-vendor 
docker run --rm \
	--user=$UID \
	-v $(go env GOCACHE):/.cache:Z \
	-v $PWD:/go/src/github.com/openshift/ci-tools:Z \
	-w /go/src/github.com/openshift/ci-tools \
	-e GO111MODULE=on \
	-e GOPROXY=https://proxy.golang.org \
	golang:1.14 \
	/bin/bash -c "go mod tidy && go mod vendor"
Trying to pull registry.fedoraproject.org/golang:1.14...
  manifest unknown: manifest unknown
Trying to pull registry.access.redhat.com/golang:1.14...
  name unknown: Repo not found
Trying to pull registry.centos.org/golang:1.14...
  manifest unknown: manifest unknown
Trying to pull docker.io/library/golang:1.14...
Getting image source signatures
Copying blob 57df1a1f1ad8 done  
Copying blob 03f1c9932170 done  
Copying blob 71e126169501 done  
Copying blob f4773b341423 done  
Copying blob 1af28a55c3f3 done  
Copying blob 6e11e1e4650d done  
Copying blob 7bdfcd218115 done  
Copying config d6747a1383 done  
Writing manifest to image destination
Storing signatures
build cache is required, but could not be located: GOCACHE is not defined and neither $XDG_CACHE_HOME nor $HOME are defined
make: *** [Makefile:89: update-vendor] Error 1

@alvaroaleman do you know how this is supposed to work?

@stevekuznetsov stevekuznetsov force-pushed the skuznets/slack-to-jira branch 2 times, most recently from 6bff56a to d3efa7c Compare October 1, 2020 01:45
@bbguimaraes
Copy link
Contributor

Would you mind adding a README with a short architectural description? It could be as simple as a list of links to the relevant API documentation if it's good.

I found it hard to even begin reviewing the interaction code without knowing what process it implements. It would also benefit other reviewers and future-us during maintenance.

@stevekuznetsov stevekuznetsov force-pushed the skuznets/slack-to-jira branch 6 times, most recently from a7d4e70 to 1e0090e Compare October 8, 2020 21:10
@stevekuznetsov
Copy link
Contributor Author

stevekuznetsov commented Oct 9, 2020 •
edited by openshift-ci-robot

This implements DPTP-1476

@stevekuznetsov stevekuznetsov force-pushed the skuznets/slack-to-jira branch 3 times, most recently from d18bcc4 to 399e367 Compare October 12, 2020 23:04
@stevekuznetsov
slack-bot: handle interactions by filing issues
0b784b9 
With this patch, the test platform Slack integration now knows how to
respond to a number of shortcut actions from users, opening dialogs that
prompt them for information in order to open issues on the DPTP Jira
board. The following types of issues are supported:
 - bugs
 - consulting requests
 - feature requests
 - incident tracking cards

Input validation is achieved with custom validation on submissiosn as
well as built-in Slack modal features; users have interactive flows to
provide them feedback on the inputs and allow them to open bugs against
this tool if we run into unexpected issues.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@bbguimaraes
Copy link
Contributor

Would you mind adding a README with a short architectural description? It could be as simple as a list of links to the relevant API documentation if it's good.

Posting the reply here for other reviewers: https://api.slack.com/surfaces/modals/using.

petr-muller
petr-muller reviewed Oct 13, 2020
View reviewed changes
Copy link
Member

@petr-muller petr-muller left a comment

Choose a reason for hiding this comment

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

I haven't gone line-by-line (surprise!) but the structure makes sense to me and I think I would be able to maintain and extend this if needed, so LGTM.

One thing that I did not like though are the tests with huge JSON blobs as inputs, I don't think they serve their purpose well, although I see how it's not really easy to make the inputs trivial and easy to read. Could we have some builder for them? And if not, I think we should at least use fixtures.

pkg/slack/util.go Outdated
@@ -0,0 +1,4 @@
package slack

// CoreOSHost is the hostname for the CoreOS Slack
Copy link
Member

Choose a reason for hiding this comment

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

nit: actually not a hostname but URL

pkg/slack/interactions/router/router.go Outdated Show resolved Hide resolved
pkg/slack/modals/bug/view_test.go Outdated Show resolved Hide resolved
@stevekuznetsov
Copy link
Contributor Author

/test e2e

@stevekuznetsov
slack-bot: change tests over to use minimal fixtures
d87a1b8 
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@stevekuznetsov
Copy link
Contributor Author

All tests use fixtures, responded to all comments. @alvaroaleman @petr-muller

petr-muller
petr-muller approved these changes Oct 14, 2020
View reviewed changes
Copy link
Member

@petr-muller petr-muller left a comment

Choose a reason for hiding this comment

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

✔️

@petr-muller
Copy link
Member

/hold

Fol Alvaro to have a chance

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: petr-muller, stevekuznetsov

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:
  • OWNERS [petr-muller,stevekuznetsov]

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

@alvaroaleman
Copy link
Contributor

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2020
@openshift-ci-robot
Copy link
Contributor

@stevekuznetsov: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e d87a1b8 link /test e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@stevekuznetsov
Copy link
Contributor Author

/test e2e

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 2650192 into openshift:master Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvaroaleman alvaroaleman alvaroaleman left review comments

@petr-muller petr-muller petr-muller approved these changes

@bbguimaraes bbguimaraes Awaiting requested review from bbguimaraes

Assignees

@petr-muller petr-muller

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@stevekuznetsov @bbguimaraes @petr-muller @openshift-ci-robot @alvaroaleman @openshift-bot @openshift-merge-robot