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

Reimplement translation prs #143

Merged
merged 22 commits into from
Jul 4, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions config/config-mattermod.default.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
"ExpectedArtifacts": 0
}
],

"EnterpriseReponame": "",
"EnterpriseTriggerReponame": "",
"EnterpriseWebappReponame": "",
Expand All @@ -66,6 +67,11 @@
"EnterpriseGithubStatusEETests": "",
"EnterpriseGithubStatusTETests": "",
"EnterpriseWorkflowName": "",

"TranslationsMattermostWebhookURL": "",
"TranslationsMattermostMessage": "",
"TranslationsBot": "",

"StartLoadtestTag": "",
"StartLoadtestMessage": "",
"CLAExclusionsList": [],
Expand Down
2 changes: 1 addition & 1 deletion server/cla.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (s *Server) checkCLA(pr *model.PullRequest) {
mlog.Int("pr n", pr.Number),
)

if contains(s.Config.CLAExclusionsList, username) {
if s.IsTrustedUser(username) {
status := &github.RepoStatus{
State: github.String(stateSuccess),
Description: github.String(fmt.Sprintf("%s excluded", username)),
Expand Down
4 changes: 4 additions & 0 deletions server/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@ type Config struct {
EnterpriseGithubStatusEETests string
EnterpriseWorkflowName string

TranslationsMattermostWebhookURL string
TranslationsMattermostMessage string
TranslationsBot string
metanerd marked this conversation as resolved.
Show resolved Hide resolved

StartLoadtestTag string
StartLoadtestMessage string

Expand Down
2 changes: 1 addition & 1 deletion server/enterprise.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
)

func (s *Server) triggerEETestsForOrgMembers(pr *model.PullRequest) {
if s.IsOrgMember(pr.Username) {
if s.IsOrgMember(pr.Username) || s.IsTrustedUser(pr.Username) {
metanerd marked this conversation as resolved.
Show resolved Hide resolved
s.triggerEnterpriseTests(pr)
}
}
Expand Down
9 changes: 9 additions & 0 deletions server/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,15 @@ func (s *Server) IsOrgMember(user string) bool {
return false
}

func (s *Server) IsTrustedUser(user string) bool {
metanerd marked this conversation as resolved.
Show resolved Hide resolved
for _, knownUser := range s.Config.CLAExclusionsList {
if user == knownUser {
return true
}
}
return false
}

func (s *Server) checkIfRefExists(pr *model.PullRequest, org string, ref string) (bool, error) {
_, r, err := s.GithubClient.Git.GetRef(context.Background(), org, pr.RepoName, ref)
if err != nil {
Expand Down
2 changes: 2 additions & 0 deletions server/pull_request.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func (s *Server) handlePullRequestEvent(event *PullRequestEvent) {
s.checkCLA(pr)
s.triggerCircleCiIfNeeded(pr)
s.addHacktoberfestLabel(pr)
s.handleTranslationPr(pr)

if pr.RepoName == s.Config.EnterpriseTriggerReponame {
s.createEnterpriseTestsPendingStatus(context.TODO(), pr)
Expand All @@ -43,6 +44,7 @@ func (s *Server) handlePullRequestEvent(event *PullRequestEvent) {
mlog.Info("PR reopened", mlog.String("repo", pr.RepoName), mlog.Int("pr", pr.Number))
s.checkCLA(pr)
s.triggerCircleCiIfNeeded(pr)
s.handleTranslationPr(pr)

if pr.RepoName == s.Config.EnterpriseTriggerReponame {
s.createEnterpriseTestsPendingStatus(context.TODO(), pr)
Expand Down
7 changes: 2 additions & 5 deletions server/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,12 @@ func TestMain(m *testing.M) {
panic(err)
}
s.Start()

defer s.Stop()
exitVal := m.Run()

os.Exit(exitVal)
}

func TestPing(t *testing.T) {
defer s.Stop()

split := strings.Split(config.ListenAddress, ":")
require.Len(t, split, 2)

Expand All @@ -68,7 +65,7 @@ func TestPing(t *testing.T) {
if err != nil {
mlog.Error("unable to find project path")
}
jsonLoader := gojsonschema.NewReferenceLoader("file://" + dir + "/schema/ping.schema.json")
jsonLoader := gojsonschema.NewReferenceLoader("file://" + dir + "/testdata/schema/ping.schema.json")

result, err := gojsonschema.Validate(jsonLoader, bytesLoader)
if err != nil {
Expand Down
File renamed without changes.
27 changes: 27 additions & 0 deletions server/translations.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package server

import (
"fmt"
"time"

"github.com/mattermost/mattermost-mattermod/model"
"github.com/mattermost/mattermost-server/v5/mlog"
)

func (s *Server) handleTranslationPr(pr *model.PullRequest) {
metanerd marked this conversation as resolved.
Show resolved Hide resolved
if pr.Username != s.Config.Username {
return
}

dataMsg := fmt.Sprintf("####[%v translations PR %v](%v)\n", pr.RepoName, time.Now().UTC().Format(time.RFC3339), pr.URL)
msg := dataMsg + s.Config.TranslationsMattermostMessage
mlog.Debug("Sending Mattermost message", mlog.String("message", msg))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest changing the message to something like Translation instead of Mattermost for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why does it make it clearer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because the payload is being sent to translation service and not Mattermost. Isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is sent to mattermost. It contains the links to the PRs for translations: https://community.mattermost.com/core/pl/sqzth7ryibgrbqj196trrs4hec


webhookRequest := &Payload{Username: "Weblate", Text: msg}
r, err := s.sendToWebhook(s.Config.TranslationsMattermostWebhookURL, webhookRequest)
if err != nil {
mlog.Error("Unable to post to Mattermost webhook", mlog.Err(err))
Copy link
Contributor

Choose a reason for hiding this comment

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

The same, so if any error happens and one looks at the logs won't get confused.

return
}
defer r.Body.Close()
metanerd marked this conversation as resolved.
Show resolved Hide resolved
}
10 changes: 4 additions & 6 deletions server/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,6 @@ import (
)

func (s *Server) logToMattermost(msg string, args ...interface{}) {
if s.Config.MattermostWebhookURL == "" {
mlog.Warn("No Mattermost webhook URL set: unable to send message")
return
}

webhookMessage := fmt.Sprintf(msg, args...)
mlog.Debug("Sending Mattermost message", mlog.String("message", webhookMessage))

Expand All @@ -21,9 +16,12 @@ func (s *Server) logToMattermost(msg string, args ...interface{}) {

webhookRequest := &Payload{Username: "Mattermod", Text: webhookMessage}

if err := s.sendToWebhook(webhookRequest); err != nil {
r, err := s.sendToWebhook(s.Config.MattermostWebhookURL, webhookRequest)
if err != nil {
mlog.Error("Unable to post to Mattermost webhook", mlog.Err(err))
return
}
defer r.Body.Close()
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as earlier.

Copy link
Member

Choose a reason for hiding this comment

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

I am not seeing this resolved. Please see my comment #143. We can just use closeBody here.

}

func NewBool(b bool) *bool { return &b }
Expand Down
47 changes: 35 additions & 12 deletions server/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package server
import (
"bytes"
"encoding/json"
"io"
"io/ioutil"
"net/http"

Expand All @@ -18,31 +17,55 @@ type Payload struct {
Text string `json:"text"`
}

func (s *Server) sendToWebhook(payload *Payload) error {
func (s *Server) sendToWebhook(webhookURL string, payload *Payload) (*http.Response, error) {
err := validateSendToWebhookRequest(webhookURL, payload)
if err != nil {
badRequestR := &http.Response{
StatusCode: http.StatusBadRequest,
Body: ioutil.NopCloser(bytes.NewBufferString(err.Error())),
}
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering the reasoning behind this? The body only contains the error string, and we are already returning the error. What extra are we including by injecting an artificially constructed response?

Same for others below.

Copy link
Member

Choose a reason for hiding this comment

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

This does not appear to be resolved?

return badRequestR, err
}

payloadBytes, err := json.Marshal(payload)
if err != nil {
return err
internalServerErrorR := &http.Response{
StatusCode: http.StatusInternalServerError,
Body: ioutil.NopCloser(bytes.NewBufferString(err.Error())),
}
return internalServerErrorR, err
}
body := bytes.NewReader(payloadBytes)

req, err := http.NewRequest(http.MethodPost, s.Config.MattermostWebhookURL, body)
req, err := http.NewRequest(http.MethodPost, webhookURL, body)
if err != nil {
return err
internalServerErrorR := &http.Response{
StatusCode: http.StatusInternalServerError,
Body: ioutil.NopCloser(bytes.NewBufferString(err.Error())),
}
return internalServerErrorR, err
}
req.Header.Set("Content-Type", "application/json")

r, err := http.DefaultClient.Do(req)
if err != nil {
return err
return r, err
}

return r, nil
}

func validateSendToWebhookRequest(webhookURL string, payload *Payload) error {
if webhookURL == "" {
return errors.New("no Mattermost webhook URL set: unable to send message")
}
defer func() {
_, _ = io.Copy(ioutil.Discard, r.Body)
r.Body.Close()
}()

if r.StatusCode != http.StatusOK {
return errors.Errorf("received non-200 status code posting to mattermost: %v, %v", r.StatusCode, r.Body)
if payload.Username == "" {
return errors.New("username not set in webhook payload")
}

if payload.Text == "" {
return errors.New("text not set in webhook payload")
}
return nil
}
81 changes: 81 additions & 0 deletions server/webhook_integration_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package server
metanerd marked this conversation as resolved.
Show resolved Hide resolved

import (
"fmt"
"net/http"
"net/http/httptest"
"testing"
)

func TestSendToWebhook(t *testing.T) {
validPayload := &Payload{Username: "mattermod", Text: "test"}

mattermost := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
fmt.Fprintln(w, "ok")
metanerd marked this conversation as resolved.
Show resolved Hide resolved
}))
defer mattermost.Close()

r, err := s.sendToWebhook(mattermost.URL, validPayload)
if err != nil {
t.Fatal(err.Error())
metanerd marked this conversation as resolved.
Show resolved Hide resolved
}
got := r.StatusCode
if got != http.StatusOK {
t.Errorf("got: %v, want: %v", got, http.StatusOK)
}
defer r.Body.Close()
metanerd marked this conversation as resolved.
Show resolved Hide resolved
}

func TestSendToWebhookUsernameNotSet(t *testing.T) {
invalidPayload := &Payload{Text: "test"}
mattermost := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
}))
defer mattermost.Close()

wantError := "username not set in webhook payload"
metanerd marked this conversation as resolved.
Show resolved Hide resolved
r, err := s.sendToWebhook(mattermost.URL, invalidPayload)
if err == nil {
t.Fatal("expected error")
}
if err.Error() != wantError {
t.Fatalf("got: %v, want: %v", err.Error(), wantError)
}

if r == nil {
t.Fatal("expected response")
}

wantStatusCode := http.StatusBadRequest
gotStatusCode := r.StatusCode
if gotStatusCode != wantStatusCode {
t.Errorf("got: %v, want: %v", gotStatusCode, wantStatusCode)
}
defer r.Body.Close()
}

func TestSendToWebhookWebhookURLNotSet(t *testing.T) {
validPayload := &Payload{Username: "mattermod", Text: "test"}
mattermost := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
}))
defer mattermost.Close()

wantError := "no Mattermost webhook URL set: unable to send message"
r, err := s.sendToWebhook("", validPayload)
if err == nil {
t.Fatal("expected error")
}
if err.Error() != wantError {
t.Fatalf("got: %v, want: %v", err.Error(), wantError)
}

if r == nil {
t.Fatal("expected response")
}

wantStatusCode := http.StatusBadRequest
gotStatusCode := r.StatusCode
if gotStatusCode != wantStatusCode {
t.Errorf("got: %v, want: %v", gotStatusCode, wantStatusCode)
}
defer r.Body.Close()
}