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 19 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
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
24 changes: 13 additions & 11 deletions server/cla.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func (s *Server) checkCLA(ctx context.Context, pr *model.PullRequest) {
mlog.Int("pr n", pr.Number),
)

if contains(s.Config.CLAExclusionsList, username) {
if s.IsBotUserFromCLAExclusionsList(username) {
status := &github.RepoStatus{
State: github.String(stateSuccess),
Description: github.String(fmt.Sprintf("%s excluded", username)),
Expand All @@ -67,7 +67,7 @@ func (s *Server) checkCLA(ctx context.Context, pr *model.PullRequest) {
return
}

body, errCSV := s.getCSV()
body, errCSV := s.getCSV(ctx)
if errCSV != nil {
return
}
Expand Down Expand Up @@ -103,19 +103,21 @@ func (s *Server) checkCLA(ctx context.Context, pr *model.PullRequest) {
_ = s.createRepoStatus(ctx, pr, status)
}

func (s *Server) getCSV() ([]byte, error) {
resp, err := http.Get(s.Config.SignedCLAURL)
func (s *Server) getCSV(ctx context.Context) ([]byte, error) {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, s.Config.SignedCLAURL, http.NoBody)
if err != nil {
mlog.Error("Unable to get CLA list", mlog.Err(err))
s.logToMattermost("unable to get CLA google csv file Error: ```" + err.Error() + "```")
return nil, err
}
defer resp.Body.Close()
r, err := http.DefaultClient.Do(req)
if err != nil {
s.logToMattermost(ctx, "unable to get CLA google csv file Error: ```"+err.Error()+"```")
return nil, err
}
agnivade marked this conversation as resolved.
Show resolved Hide resolved
closeBody(r)
metanerd marked this conversation as resolved.
Show resolved Hide resolved

body, err := ioutil.ReadAll(resp.Body)
body, err := ioutil.ReadAll(r.Body)
if err != nil {
mlog.Error("Unable to read response body", mlog.Err(err))
s.logToMattermost("unable to read CLA google csv file Error: ```" + err.Error() + "```")
s.logToMattermost(ctx, "unable to read CLA google csv file Error: ```"+err.Error()+"```")
return nil, err
}
return body, nil
Expand Down Expand Up @@ -150,6 +152,6 @@ func (s *Server) createCLAPendingStatus(ctx context.Context, pr *model.PullReque
}
err := s.createRepoStatus(ctx, pr, status)
if err != nil {
s.logToMattermost("failed to create status for PR: " + strconv.Itoa(pr.Number) + " Context: " + s.Config.CLAGithubStatusContext + " Error: ```" + err.Error() + "```")
s.logToMattermost(ctx, "failed to create status for PR: "+strconv.Itoa(pr.Number)+" Context: "+s.Config.CLAGithubStatusContext+" Error: ```"+err.Error()+"```")
}
}
4 changes: 4 additions & 0 deletions server/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,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
4 changes: 2 additions & 2 deletions 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.IsBotUserFromCLAExclusionsList(pr.Username) {
s.triggerEnterpriseTests(pr)
}
}
Expand Down Expand Up @@ -142,7 +142,7 @@ func (s *Server) createEnterpriseTestsPendingStatus(ctx context.Context, pr *mod
}
err := s.createRepoStatus(ctx, pr, enterpriseStatus)
if err != nil {
s.logToMattermost("failed to create status for PR: " + strconv.Itoa(pr.Number) + " Context: " + s.Config.EnterpriseGithubStatusContext + " Error: ```" + err.Error() + "```")
s.logToMattermost(ctx, "failed to create status for PR: "+strconv.Itoa(pr.Number)+" Context: "+s.Config.EnterpriseGithubStatusContext+" Error: ```"+err.Error()+"```")
}
}

Expand Down
9 changes: 9 additions & 0 deletions server/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,15 @@ func (s *Server) IsOrgMember(user string) bool {
return false
}

func (s *Server) IsBotUserFromCLAExclusionsList(user string) bool {
for _, claExcludedUser := range s.Config.CLAExclusionsList {
if user == claExcludedUser {
return true
}
}
return false
}

func (s *Server) checkIfRefExists(ctx context.Context, pr *model.PullRequest, org string, ref string) (bool, error) {
_, r, err := s.GithubClient.Git.GetRef(ctx, 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(ctx context.Context, event *PullRequestE
s.checkCLA(ctx, pr)
s.triggerCircleCiIfNeeded(ctx, pr)
s.addHacktoberfestLabel(ctx, pr)
s.handleTranslationPR(ctx, pr)

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

if pr.RepoName == s.Config.EnterpriseTriggerReponame {
s.createEnterpriseTestsPendingStatus(ctx, pr)
Expand Down
14 changes: 11 additions & 3 deletions server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"encoding/json"
"errors"
"fmt"
"io"
"io/ioutil"
"net/http"
"os"
Expand Down Expand Up @@ -125,14 +126,14 @@ func (s *Server) RefreshMembers() {
members, err := s.getMembers(ctx)
if err != nil {
mlog.Error("failed to refresh org members", mlog.Err(err))
s.logToMattermost("refresh failed, using org members of previous day\n" + err.Error())
s.logToMattermost(ctx, "refresh failed, using org members of previous day\n"+err.Error())
return
}

if members == nil {
err = errors.New("no members found")
mlog.Error("failed to refresh org members", mlog.Err(err))
s.logToMattermost("refresh failed, using org members of previous day\n" + err.Error())
s.logToMattermost(ctx, "refresh failed, using org members of previous day\n"+err.Error())
return
}

Expand Down Expand Up @@ -209,7 +210,7 @@ func (s *Server) githubEvent(w http.ResponseWriter, r *http.Request) {
stopRequests, timeUntilReset := s.shouldStopRequests(ctx)
if stopRequests {
if !s.hasReportedRateLimit && timeUntilReset != nil {
s.logToMattermost(":warning: Hit rate limit. Time until reset: " + timeUntilReset.String())
s.logToMattermost(ctx, ":warning: Hit rate limit. Time until reset: "+timeUntilReset.String())
}
return
}
Expand Down Expand Up @@ -305,3 +306,10 @@ func SetupLogging(config *Config) {
mlog.RedirectStdLog(logger)
mlog.InitGlobalLogger(logger)
}

func closeBody(r *http.Response) {
if r.Body != nil {
_, _ = io.Copy(ioutil.Discard, r.Body)
_ = r.Body.Close()
}
}
4 changes: 2 additions & 2 deletions server/spinmint.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func (s *Server) waitForBuildAndSetupSpinmint(pr *model.PullRequest, upgradeServ
var errInstance error
instance, errInstance = s.setupSpinmint(ctx, pr, repo, upgradeServer)
if errInstance != nil {
s.logToMattermost("Unable to set up spinmint for PR %v in %v/%v: %v", pr.Number, pr.RepoOwner, pr.RepoName, errInstance.Error())
s.logToMattermost(ctx, "Unable to set up spinmint for PR %v in %v/%v: %v", pr.Number, pr.RepoOwner, pr.RepoName, errInstance.Error())
s.sendGitHubComment(ctx, pr.RepoOwner, pr.RepoName, pr.Number, s.Config.SetupSpinmintFailedMessage)
return
}
Expand All @@ -75,7 +75,7 @@ func (s *Server) waitForBuildAndSetupSpinmint(pr *model.PullRequest, upgradeServ
publicDNS, internalIP := s.getIPsForInstance(ctx, *instance.InstanceId)

if err := s.updateRoute53Subdomain(ctx, *instance.InstanceId, publicDNS, "CREATE"); err != nil {
s.logToMattermost("Unable to set up S3 subdomain for PR %v in %v/%v with instance %v: %v", pr.Number, pr.RepoOwner, pr.RepoName, *instance.InstanceId, err.Error())
s.logToMattermost(ctx, "Unable to set up S3 subdomain for PR %v in %v/%v with instance %v: %v", pr.Number, pr.RepoOwner, pr.RepoName, *instance.InstanceId, err.Error())
s.sendGitHubComment(ctx, pr.RepoOwner, pr.RepoName, pr.Number, s.Config.SetupSpinmintFailedMessage)
return
}
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 (
"context"
"fmt"
"time"

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

func (s *Server) handleTranslationPR(ctx context.Context, pr *model.PullRequest) {
if pr.Username != s.Config.TranslationsBot {
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}
err := s.sendToWebhook(ctx, 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
}
}
12 changes: 5 additions & 7 deletions server/utils.go
Original file line number Diff line number Diff line change
@@ -1,17 +1,13 @@
package server

import (
"context"
"fmt"

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

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

func (s *Server) logToMattermost(ctx context.Context, msg string, args ...interface{}) {
webhookMessage := fmt.Sprintf(msg, args...)
mlog.Debug("Sending Mattermost message", mlog.String("message", webhookMessage))

Expand All @@ -21,8 +17,10 @@ func (s *Server) logToMattermost(msg string, args ...interface{}) {

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

if err := s.sendToWebhook(webhookRequest); err != nil {
err := s.sendToWebhook(ctx, s.Config.MattermostWebhookURL, webhookRequest)
if err != nil {
mlog.Error("Unable to post to Mattermost webhook", mlog.Err(err))
return
}
}

Expand Down
50 changes: 38 additions & 12 deletions server/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,45 @@ package server

import (
"bytes"
"context"
"encoding/json"
"io"
"io/ioutil"
"net/http"

"github.com/pkg/errors"
)

// WebhookValidationError contains an error in the webhook payload.
type WebhookValidationError struct {
field string
}

// Error implements the error interface.
func (e *WebhookValidationError) Error() string {
return "invalid" + e.field
agnivade marked this conversation as resolved.
Show resolved Hide resolved
}

func newWebhookValidationError(field string) *WebhookValidationError {
return &WebhookValidationError{
field: field,
}
}

type Payload struct {
Username string `json:"username"`
Text string `json:"text"`
}

func (s *Server) sendToWebhook(payload *Payload) error {
func (s *Server) sendToWebhook(ctx context.Context, webhookURL string, payload *Payload) error {
err := validateSendToWebhookRequest(webhookURL, payload)
if err != nil {
return err
}

payloadBytes, err := json.Marshal(payload)
if err != nil {
return err
}
body := bytes.NewReader(payloadBytes)

req, err := http.NewRequest(http.MethodPost, s.Config.MattermostWebhookURL, body)
req, err := http.NewRequestWithContext(ctx, http.MethodPost, webhookURL, body)
if err != nil {
return err
}
Expand All @@ -35,14 +53,22 @@ func (s *Server) sendToWebhook(payload *Payload) error {
if err != nil {
return err
}
defer func() {
_, _ = io.Copy(ioutil.Discard, r.Body)
r.Body.Close()
}()
closeBody(r)

if r.StatusCode != http.StatusOK {
return errors.Errorf("received non-200 status code posting to mattermost: %v, %v", r.StatusCode, r.Body)
return nil
}

func validateSendToWebhookRequest(webhookURL string, payload *Payload) error {
if webhookURL == "" {
return newWebhookValidationError("webhook URL")
}

if payload.Username == "" {
return newWebhookValidationError("username")
}

if payload.Text == "" {
return &WebhookValidationError{"text"}
agnivade marked this conversation as resolved.
Show resolved Hide resolved
}
return nil
}
52 changes: 52 additions & 0 deletions server/webhook_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package server

import (
"context"
"errors"
"net/http"
"net/http/httptest"
"testing"

Copy link
Member

Choose a reason for hiding this comment

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

Nit: remove gap

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestSendToWebhookIntegration(t *testing.T) {
s := &Server{}

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

err := s.sendToWebhook(context.Background(), mattermost.URL, validPayload)
require.NoError(t, err)
}

func TestSendToWebhookUsernameNotSetIntegration(t *testing.T) {
s := &Server{}

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

err := s.sendToWebhook(context.Background(), mattermost.URL, invalidPayload)
var whError *WebhookValidationError
require.True(t, errors.As(err, &whError))
assert.Equal(t, whError.field, "username")
}

func TestSendToWebhookWebhookURLNotSetIntegration(t *testing.T) {
s := &Server{}

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

err := s.sendToWebhook(context.Background(), "", validPayload)
var whError *WebhookValidationError
require.True(t, errors.As(err, &whError))
assert.Equal(t, whError.field, "webhook URL")
}