Merge changes I8b30dbdf,I32736bca,Ifd81ea7a,I504dfe7b,I96d490b4, ...
* changes:
changelog-webapp: Fixed changelog form alignment
changelog-webapp: Renamed locatebuild
changelog-webapp: Login redirect on completion
changelog: Test error codes
findbuild: Handle early Chromium builds
changelog-webapp: Added signout
changelog: Added caveat info
changelog: Added error interface and utils
changelog: Use ChangelogError errors
changelog-webapp: Link creation using new changelog package
changelog: Support multiple repos with same name
diff --git a/go.mod b/go.mod
index b915599..a5960a0 100644
--- a/go.mod
+++ b/go.mod
@@ -19,7 +19,6 @@
go.chromium.org/luci v0.0.0-20200722211809-bab0c30be68b
golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d
google.golang.org/api v0.28.0
- google.golang.org/grpc v1.29.1
google.golang.org/genproto v0.0.0-20200618031413-b414f8b61790
google.golang.org/grpc v1.29.1
google.golang.org/protobuf v1.25.0
diff --git a/src/cmd/changelog-webapp/controllers/authHandlers.go b/src/cmd/changelog-webapp/controllers/authHandlers.go
index aa159ee..2a4a983 100644
--- a/src/cmd/changelog-webapp/controllers/authHandlers.go
+++ b/src/cmd/changelog-webapp/controllers/authHandlers.go
@@ -16,7 +16,6 @@
import (
"context"
- "errors"
"fmt"
"math/rand"
"net/http"
@@ -35,7 +34,6 @@
// Session variables
sessionName = "changelog"
sessionKeyLength = 32
- sessionAge = 84600
// Oauth state generation variables
oauthStateCharset = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz1234567890"
@@ -54,27 +52,22 @@
var clientSecretName = os.Getenv("COS_CHANGELOG_CLIENT_SECRET_NAME")
var sessionSecretName = os.Getenv("COS_CHANGELOG_SESSION_SECRET_NAME")
-// ErrorSessionRetrieval indicates that a request has no session, or the
-// session was malformed.
-var ErrorSessionRetrieval = errors.New("No session found")
-
func init() {
var err error
client, err := secretmanager.NewClient(context.Background())
if err != nil {
- log.Fatalf("Failed to setup client: %v", err)
+ log.Fatalf("failed to setup client: %v", err)
}
config.ClientSecret, err = getSecret(client, clientSecretName)
if err != nil {
- log.Fatalf("Failed to retrieve secret: %s\n%v", clientSecretName, err)
+ log.Fatalf("failed to retrieve secret: %s\n%v", clientSecretName, err)
}
sessionSecret, err := getSecret(client, sessionSecretName)
if err != nil {
- log.Fatalf("Failed to retrieve secret :%s\n%v", sessionSecretName, err)
+ log.Fatalf("failed to retrieve secret :%s\n%v", sessionSecretName, err)
}
store = sessions.NewCookieStore([]byte(sessionSecret))
- store.MaxAge(sessionAge)
}
// Retrieve secrets stored in Gcloud Secret Manager
@@ -84,7 +77,7 @@
}
result, err := client.AccessSecretVersion(context.Background(), accessRequest)
if err != nil {
- return "", fmt.Errorf("Failed to access secret at %s: %v", accessRequest.Name, err)
+ return "", fmt.Errorf("failed to access secret at %s: %v", accessRequest.Name, err)
}
return string(result.Payload.Data), nil
}
@@ -102,27 +95,60 @@
return state[oauthStateLength:]
}
-// HTTPClient creates an authorized HTTP Client using stored token credentials.
-// Returns error if no session or a malformed session is detected.
-// Otherwise returns an HTTP Client with the stored Oauth access token.
-// If the access token is expired, automatically refresh the token
-func HTTPClient(w http.ResponseWriter, r *http.Request, returnURL string) (*http.Client, error) {
+// tokenExpired indicates whether the Oauth token associated with a request is expired
+func tokenExpired(r *http.Request) bool {
var parsedExpiry time.Time
+ session, _ := store.Get(r, sessionName)
+ parsedExpiry, err := time.Parse(time.RFC3339, session.Values["expiry"].(string))
+ return err != nil || parsedExpiry.Before(time.Now())
+}
+
+// GetLoginURL returns a login URL to redirect the user to
+func GetLoginURL(redirect string, auto bool) string {
+ return fmt.Sprintf("/login/?redirect=%s&auto=%v", redirect, auto)
+}
+
+// SignedIn returns a bool indicating if the current request is signed in
+func SignedIn(r *http.Request) bool {
session, err := store.Get(r, sessionName)
if err != nil || session.IsNew {
- return nil, ErrorSessionRetrieval
+ return false
}
for _, key := range []string{"accessToken", "refreshToken", "tokenType", "expiry"} {
if val, ok := session.Values[key]; !ok || val == nil {
- return nil, ErrorSessionRetrieval
+ return false
}
}
- if parsedExpiry, err = time.Parse(time.RFC3339, session.Values["expiry"].(string)); err != nil {
- return nil, ErrorSessionRetrieval
+ return true
+}
+
+// RequireToken will check if the user has a valid, unexpired Oauth token.
+// If not, it will initiate the Oauth flow.
+// Returns a bool indicating if the user was redirected
+func RequireToken(w http.ResponseWriter, r *http.Request, activePage string) bool {
+ if !SignedIn(r) {
+ err := promptLoginTemplate.Execute(w, &statusPage{ActivePage: activePage})
+ if err != nil {
+ log.Errorf("error executing promptLogin template: %v", err)
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ }
+ return true
}
- if parsedExpiry.Before(time.Now()) {
- log.Debug("HTTPClient: Token expired, calling Oauth flow")
- HandleLogin(w, r, returnURL)
+ // If token is expired, auto refresh instead of prompting sign in
+ if tokenExpired(r) {
+ loginURL := GetLoginURL(activePage, true)
+ http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
+ return true
+ }
+ return false
+}
+
+// HTTPClient creates an authorized HTTP Client using stored token credentials.
+func HTTPClient(w http.ResponseWriter, r *http.Request) (*http.Client, error) {
+ session, _ := store.Get(r, sessionName)
+ parsedExpiry, err := time.Parse(time.RFC3339, session.Values["expiry"].(string))
+ if err != nil {
+ return nil, err
}
token := &oauth2.Token{
AccessToken: session.Values["accessToken"].(string),
@@ -134,21 +160,36 @@
}
// HandleLogin initiates the Oauth flow.
-func HandleLogin(w http.ResponseWriter, r *http.Request, returnURL string) {
- state := randomString(oauthStateLength, returnURL)
- // Ignore store.Get() errors in HandleLogin because an error indicates the
+func HandleLogin(w http.ResponseWriter, r *http.Request) {
+ if err := r.ParseForm(); err != nil {
+ log.Errorf("could not parse request: %v\n", err)
+ http.Error(w, err.Error(), http.StatusBadRequest)
+ return
+ }
+ autoAuth := r.FormValue("auto") == "true"
+ redirect := r.FormValue("redirect")
+ if redirect == "" {
+ redirect = "/"
+ }
+
+ state := randomString(oauthStateLength, redirect)
+ // Ignore store.Get() errors because an error indicates the
// old session could not be deciphered. It returns a new session
// regardless.
session, _ := store.Get(r, sessionName)
session.Values["oauthState"] = state
err := session.Save(r, w)
if err != nil {
- log.Errorf("HandleLogin: Error saving key: %v", err)
+ log.Errorf("Error saving key: %v", err)
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
-
- authURL := config.AuthCodeURL(state, oauth2.AccessTypeOffline)
+ var authURL string
+ if autoAuth {
+ authURL = config.AuthCodeURL(state, oauth2.AccessTypeOffline)
+ } else {
+ authURL = config.AuthCodeURL(state, oauth2.AccessTypeOffline, oauth2.ApprovalForce)
+ }
http.Redirect(w, r, authURL, http.StatusTemporaryRedirect)
}
@@ -157,7 +198,7 @@
// Redirects to URL stored in the callback state on completion.
func HandleCallback(w http.ResponseWriter, r *http.Request) {
if err := r.ParseForm(); err != nil {
- log.Errorf("Could not parse query: %v\n", err)
+ log.Errorf("could not parse query: %v\n", err)
http.Error(w, err.Error(), http.StatusBadRequest)
return
}
@@ -166,7 +207,7 @@
session, err := store.Get(r, sessionName)
if err != nil {
- log.Errorf("HandleCallback: Error retrieving session: %v", err)
+ log.Errorf("error retrieving session: %v", err)
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
@@ -182,7 +223,7 @@
token, err := config.Exchange(context.Background(), authCode)
if err != nil {
- log.Errorf("HandleCallback: Error exchanging token: %v", token)
+ log.Errorf("error exchanging token: %v", token)
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
@@ -193,10 +234,34 @@
session.Values["expiry"] = token.Expiry.Format(time.RFC3339)
err = session.Save(r, w)
if err != nil {
- log.Errorf("HandleCallback: Error saving session: %v", err)
+ log.Errorf("error saving session: %v", err)
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
+ http.Redirect(w, r, returnURLFromState(sessionState), http.StatusTemporaryRedirect)
+}
- http.Redirect(w, r, returnURLFromState(sessionState), http.StatusPermanentRedirect)
+// HandleSignOut signs out the user by removing token information from the
+// session
+func HandleSignOut(w http.ResponseWriter, r *http.Request) {
+ if err := r.ParseForm(); err != nil {
+ log.Errorf("could not parse request: %v\n", err)
+ http.Error(w, err.Error(), http.StatusBadRequest)
+ return
+ }
+ redirect := r.FormValue("redirect")
+ if redirect == "" {
+ redirect = "/"
+ }
+ session, _ := store.Get(r, sessionName)
+ session.Values["accessToken"] = nil
+ session.Values["refreshToken"] = nil
+ session.Values["tokenType"] = nil
+ session.Values["expiry"] = nil
+ err := session.Save(r, w)
+ if err != nil {
+ log.Errorf("error saving session: %v", err)
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ }
+ http.Redirect(w, r, redirect, http.StatusTemporaryRedirect)
}
diff --git a/src/cmd/changelog-webapp/controllers/pageHandlers.go b/src/cmd/changelog-webapp/controllers/pageHandlers.go
index 87eb7d9..c0faf97 100644
--- a/src/cmd/changelog-webapp/controllers/pageHandlers.go
+++ b/src/cmd/changelog-webapp/controllers/pageHandlers.go
@@ -16,11 +16,9 @@
import (
"context"
- "errors"
"fmt"
"net/http"
"os"
- "regexp"
"strconv"
"strings"
"text/template"
@@ -28,8 +26,7 @@
secretmanager "cloud.google.com/go/secretmanager/apiv1"
"cos.googlesource.com/cos/tools/src/pkg/changelog"
"cos.googlesource.com/cos/tools/src/pkg/findbuild"
- "google.golang.org/grpc/codes"
- "google.golang.org/grpc/status"
+ "cos.googlesource.com/cos/tools/src/pkg/utils"
log "github.com/sirupsen/logrus"
)
@@ -54,43 +51,9 @@
indexTemplate *template.Template
changelogTemplate *template.Template
promptLoginTemplate *template.Template
- locateBuildTemplate *template.Template
+ findBuildTemplate *template.Template
statusForbiddenTemplate *template.Template
basicTextTemplate *template.Template
-
- grpcCodeToHTTP = map[string]string{
- codes.Unknown.String(): "500",
- codes.InvalidArgument.String(): "400",
- codes.NotFound.String(): "404",
- codes.PermissionDenied.String(): "403",
- codes.Unauthenticated.String(): "401",
- codes.ResourceExhausted.String(): "429",
- codes.FailedPrecondition.String(): "400",
- codes.OutOfRange.String(): "400",
- codes.Internal.String(): "500",
- codes.Unavailable.String(): "503",
- codes.DataLoss.String(): "500",
- }
- httpCodeToHeader = map[string]string{
- "400": "400 Bad Request",
- "401": "401 Unauthorized",
- "403": "403 Forbidden",
- "404": "404 Not Found",
- "429": "429 Too Many Requests",
- "500": "500 Internal Server Error",
- "503": "503 Service Unavailable",
- }
- httpCodeToDesc = map[string]string{
- "400": "The request could not be understood.",
- "401": "You are currently unauthenticated. Please login to access this resource.",
- "403": "This account does not have access to internal repositories. Please retry with an authorized account, or select the external button to query from publically accessible builds.",
- "404": "The requested resource was not found. Please verify that you are using a valid input.",
- "429": "Our servers are currently experiencing heavy load. Please retry in a couple minutes.",
- "500": "Something went wrong on our end. Please try again later.",
- "503": "This service is temporarily offline. Please try again later.",
- }
- gitiles403Desc = "unexpected HTTP 403 from Gitiles"
- gerritErrCodeRe = regexp.MustCompile("status code\\s*(\\d+)")
)
func init() {
@@ -124,7 +87,7 @@
staticBasePath = os.Getenv("STATIC_BASE_PATH")
indexTemplate = template.Must(template.ParseFiles(staticBasePath + "templates/index.html"))
changelogTemplate = template.Must(template.ParseFiles(staticBasePath + "templates/changelog.html"))
- locateBuildTemplate = template.Must(template.ParseFiles(staticBasePath + "templates/locateBuild.html"))
+ findBuildTemplate = template.Must(template.ParseFiles(staticBasePath + "templates/findBuild.html"))
promptLoginTemplate = template.Must(template.ParseFiles(staticBasePath + "templates/promptLogin.html"))
basicTextTemplate = template.Must(template.ParseFiles(staticBasePath + "templates/error.html"))
}
@@ -146,22 +109,24 @@
Internal bool
}
-type locateBuildPage struct {
- CL string
- CLNum string
- BuildNum string
- GerritLink string
- Internal bool
+type findBuildPage struct {
+ CL string
+ CLNum string
+ BuildNum string
+ GerritLink string
+ Internal bool
}
type statusPage struct {
ActivePage string
+ SignedIn bool
}
type basicTextPage struct {
Header string
Body string
ActivePage string
+ SignedIn bool
}
type repoTable struct {
@@ -198,29 +163,24 @@
func getIntVerifiedEnv(envName string) string {
output := os.Getenv(envName)
if _, err := strconv.Atoi(output); err != nil {
- log.Errorf("getEnvAsInt: Failed to parse env variable %s with value %s: %v",
+ log.Errorf("failed to parse env variable %s with value %s: %v",
envName, os.Getenv(output), err)
}
return output
}
-func unwrappedError(err error) error {
- innerErr := err
- for errors.Unwrap(innerErr) != nil {
- innerErr = errors.Unwrap(innerErr)
- }
- return innerErr
-}
-
func gobCommitLink(instance, repo, SHA string) string {
return fmt.Sprintf("https://%s/%s/+/%s", instance, repo, SHA)
}
-func gobDiffLink(instance, repo, sourceSHA, targetSHA string) string {
+func gobDiffLink(instance, repo, sourceSHA, targetSHA string, diffLink bool) string {
+ if !diffLink {
+ return fmt.Sprintf("https://%s/%s/+log/%s?n=10000", instance, repo, targetSHA)
+ }
return fmt.Sprintf("https://%s/%s/+log/%s..%s?n=10000", instance, repo, sourceSHA, targetSHA)
}
-func createRepoTableEntry(instance string, repo string, commit *changelog.Commit, isAddition bool) *repoTableEntry {
+func createRepoTableEntry(instance, repo string, commit *changelog.Commit, isAddition bool) *repoTableEntry {
entry := new(repoTableEntry)
entry.IsAddition = isAddition
entry.SHA = &shaAttr{Name: commit.SHA[:8], URL: gobCommitLink(instance, repo, commit.SHA)}
@@ -242,45 +202,47 @@
func createChangelogPage(data changelogData) *changelogPage {
page := &changelogPage{Source: data.Source, Target: data.Target, QuerySize: envQuerySize, Internal: data.Internal}
- for repoName, repoLog := range data.Additions {
- table := &repoTable{Name: repoName}
- for _, commit := range repoLog.Commits {
- tableEntry := createRepoTableEntry(data.Instance, repoName, commit, true)
+ for repoPath, addLog := range data.Additions {
+ diffLink := false
+ table := &repoTable{Name: repoPath}
+ for _, commit := range addLog.Commits {
+ tableEntry := createRepoTableEntry(data.Instance, addLog.Repo, commit, true)
table.Additions = append(table.Additions, tableEntry)
}
- if _, ok := data.Removals[repoName]; ok {
- for _, commit := range data.Removals[repoName].Commits {
- tableEntry := createRepoTableEntry(data.Instance, repoName, commit, false)
+ if rmLog, ok := data.Removals[repoPath]; ok {
+ for _, commit := range data.Removals[repoPath].Commits {
+ tableEntry := createRepoTableEntry(data.Instance, rmLog.Repo, commit, false)
table.Removals = append(table.Removals, tableEntry)
}
- if data.Removals[repoName].HasMoreCommits {
- table.RemovalsLink = gobDiffLink(data.Instance, repoName, repoLog.TargetSHA, repoLog.SourceSHA)
+ if data.Removals[repoPath].HasMoreCommits {
+ diffLink = addLog.Repo == rmLog.Repo
+ table.RemovalsLink = gobDiffLink(data.Instance, rmLog.Repo, addLog.TargetSHA, rmLog.TargetSHA, diffLink)
}
}
- if repoLog.HasMoreCommits {
- table.AdditionsLink = gobDiffLink(data.Instance, repoName, repoLog.SourceSHA, repoLog.TargetSHA)
+ if addLog.HasMoreCommits {
+ table.AdditionsLink = gobDiffLink(data.Instance, addLog.Repo, addLog.SourceSHA, addLog.TargetSHA, diffLink)
}
page.RepoTables = append(page.RepoTables, table)
}
// Add remaining repos that had removals but no additions
- for repoName, repoLog := range data.Removals {
- if _, ok := data.Additions[repoName]; ok {
+ for repoPath, repoLog := range data.Removals {
+ if _, ok := data.Additions[repoPath]; ok {
continue
}
- table := &repoTable{Name: repoName}
+ table := &repoTable{Name: repoPath}
for _, commit := range repoLog.Commits {
- tableEntry := createRepoTableEntry(data.Instance, repoName, commit, false)
+ tableEntry := createRepoTableEntry(data.Instance, repoLog.Repo, commit, false)
table.Removals = append(table.Removals, tableEntry)
}
page.RepoTables = append(page.RepoTables, table)
if repoLog.HasMoreCommits {
- table.RemovalsLink = gobDiffLink(data.Instance, repoName, repoLog.TargetSHA, repoLog.SourceSHA)
+ table.RemovalsLink = gobDiffLink(data.Instance, repoLog.Repo, repoLog.SourceSHA, repoLog.TargetSHA, false)
}
}
return page
}
-func findBuildWithFallback(httpClient *http.Client, gerrit, fallbackGerrit, gob, repo, cl string, internal bool) (*findbuild.BuildResponse, bool, error) {
+func findBuildWithFallback(httpClient *http.Client, gerrit, fallbackGerrit, gob, repo, cl string, internal bool) (*findbuild.BuildResponse, bool, utils.ChangelogError) {
didFallback := false
request := &findbuild.BuildRequest{
HTTPClient: httpClient,
@@ -291,8 +253,7 @@
CL: cl,
}
buildData, err := findbuild.FindBuild(request)
- innerErr := unwrappedError(err)
- if innerErr == findbuild.ErrorCLNotFound {
+ if err != nil && err.HTTPCode() == "404" {
log.Debugf("Cl %s not found in Gerrit instance, using fallback", cl)
fallbackRequest := &findbuild.BuildRequest{
HTTPClient: httpClient,
@@ -308,88 +269,40 @@
return buildData, didFallback, err
}
-// parseGRPCError retrieves the header and description to display for a gRPC error
-func parseGRPCError(inputErr error) (string, string, error) {
- rpcStatus, ok := status.FromError(inputErr)
- if !ok {
- return "", "", fmt.Errorf("parseGRPCError: Error %v is not a gRPC error", inputErr)
- }
- code, text := rpcStatus.Code(), rpcStatus.Message()
- // RPC status code misclassifies 403 error as 500 error for Gitiles requests
- if text == gitiles403Desc {
- code = codes.PermissionDenied
- }
- if httpCode, ok := grpcCodeToHTTP[code.String()]; ok {
- if _, ok := httpCodeToHeader[httpCode]; !ok {
- log.Errorf("parseGRPCError: No error header mapping found for HTTP code %s", httpCode)
- return "", "", fmt.Errorf("parseGRPCError: No error header mapping found for HTTP code %s", httpCode)
- }
- if _, ok := httpCodeToDesc[httpCode]; !ok {
- log.Errorf("parseGRPCError: No error description mapping found for HTTP code %s", httpCode)
- return "", "", fmt.Errorf("parseGRPCError: No error description mapping found for HTTP code %s", httpCode)
- }
- return httpCodeToHeader[httpCode], httpCodeToDesc[httpCode], nil
- }
- return "", "", fmt.Errorf("parseGRPCError: gRPC error code %s not supported", code.String())
-}
-
-// parseGitilesError retrieves the header and description to display for a Gerrit error
-func parseGerritError(inputErr error) (string, string, error) {
- matches := gerritErrCodeRe.FindStringSubmatch(inputErr.Error())
- if len(matches) != 2 {
- return "", "", fmt.Errorf("parseGerritError: error %v is not a Gerrit error", inputErr)
- }
- httpCode := matches[1]
- if _, ok := httpCodeToHeader[httpCode]; !ok {
- log.Errorf("parseGerritError: No error header mapping found for HTTP code %s", httpCode)
- return "", "", fmt.Errorf("parseGerritError: No error header mapping found for HTTP code %s", httpCode)
- }
- if _, ok := httpCodeToDesc[httpCode]; !ok {
- log.Errorf("parseGerritError: No error description mapping found for HTTP code %s", httpCode)
- return "", "", fmt.Errorf("parseGerritError: No error description mapping found for HTTP code %s", httpCode)
- }
- return httpCodeToHeader[httpCode], httpCodeToDesc[httpCode], nil
-}
-
// handleError creates the error page for a given error
-func handleError(w http.ResponseWriter, inputErr error, currPage string) {
- innerErr := unwrappedError(inputErr)
- header := "An error occurred while handling this request"
- body := innerErr.Error()
- if tmpHeader, tmpBody, err := parseGRPCError(innerErr); err == nil {
- header = tmpHeader
- body = tmpBody
- } else if tmpHeader, tmpBody, err := parseGerritError(innerErr); err == nil {
- header = tmpHeader
- body = tmpBody
- }
- basicTextTemplate.Execute(w, &basicTextPage{
- Header: header,
- Body: body,
+func handleError(w http.ResponseWriter, r *http.Request, displayErr utils.ChangelogError, currPage string) {
+ err := basicTextTemplate.Execute(w, &basicTextPage{
+ Header: displayErr.HTTPStatus(),
+ Body: displayErr.Error(),
ActivePage: currPage,
+ SignedIn: SignedIn(r),
})
+ if err != nil {
+ log.Error(err)
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ }
}
// HandleIndex serves the home page
func HandleIndex(w http.ResponseWriter, r *http.Request) {
- indexTemplate.Execute(w, nil)
+ err := indexTemplate.Execute(w, &statusPage{SignedIn: SignedIn(r)})
+ if err != nil {
+ log.Error(err)
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ }
}
// HandleChangelog serves the changelog page
func HandleChangelog(w http.ResponseWriter, r *http.Request) {
- httpClient, err := HTTPClient(w, r, "/changelog/")
- if err != nil {
- log.Debug(err)
- err = promptLoginTemplate.Execute(w, &statusPage{ActivePage: "changelog"})
- if err != nil {
- log.Errorf("HandleChangelog: error executing promptLogin template: %v", err)
- }
+ if RequireToken(w, r, "/changelog/") {
return
}
+ var err error
if err := r.ParseForm(); err != nil {
err = changelogTemplate.Execute(w, &changelogPage{QuerySize: envQuerySize})
if err != nil {
- log.Errorf("HandleChangelog: error executing locatebuild template: %v", err)
+ log.Errorf("error executing findbuild template: %v", err)
+ http.Error(w, err.Error(), http.StatusInternalServerError)
}
return
}
@@ -399,7 +312,8 @@
if source == "" || target == "" {
err = changelogTemplate.Execute(w, &changelogPage{QuerySize: envQuerySize, Internal: true})
if err != nil {
- log.Errorf("HandleChangelog: error executing locatebuild template: %v", err)
+ log.Errorf("error executing findbuild template: %v", err)
+ http.Error(w, err.Error(), http.StatusInternalServerError)
}
return
}
@@ -411,11 +325,17 @@
if r.FormValue("internal") == "true" {
internal, instance, manifestRepo = true, internalGoBInstance, internalManifestRepo
}
- added, removed, err := changelog.Changelog(httpClient, source, target, instance, manifestRepo, querySize)
+ httpClient, err := HTTPClient(w, r)
if err != nil {
- log.Errorf("HandleChangelog: error retrieving changelog between builds %s and %s on GoB instance: %s with manifest repository: %s\n%v\n",
- source, target, externalGoBInstance, externalManifestRepo, err)
- handleError(w, err, "/changelog/")
+ loginURL := GetLoginURL("/changelog/", false)
+ http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
+ return
+ }
+ added, removed, utilErr := changelog.Changelog(httpClient, source, target, instance, manifestRepo, querySize)
+ if utilErr != nil {
+ log.Errorf("error retrieving changelog between builds %s and %s on GoB instance: %s with manifest repository: %s\n%v\n",
+ source, target, externalGoBInstance, externalManifestRepo, utilErr)
+ handleError(w, r, utilErr, "/changelog/")
return
}
page := createChangelogPage(changelogData{
@@ -428,35 +348,32 @@
})
err = changelogTemplate.Execute(w, page)
if err != nil {
- log.Errorf("HandleChangelog: error executing changelog template: %v", err)
+ log.Errorf("error executing changelog template: %v", err)
+ http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
-// HandleLocateBuild serves the Locate CL page
-func HandleLocateBuild(w http.ResponseWriter, r *http.Request) {
- httpClient, err := HTTPClient(w, r, "/locatebuild/")
- // Require login to access if no session found
- if err != nil {
- log.Debug(err)
- err = promptLoginTemplate.Execute(w, &statusPage{ActivePage: "locatebuild"})
- if err != nil {
- log.Errorf("HandleLocateBuild: error executing promptLogin template: %v", err)
- }
+// HandleFindBuild serves the Locate CL page
+func HandleFindBuild(w http.ResponseWriter, r *http.Request) {
+ if RequireToken(w, r, "/findbuild/") {
return
}
- if err := r.ParseForm(); err != nil {
- err = locateBuildTemplate.Execute(w, &locateBuildPage{Internal: true})
+ var err error
+ if err = r.ParseForm(); err != nil {
+ err = findBuildTemplate.Execute(w, &findBuildPage{Internal: true})
if err != nil {
- log.Errorf("HandleLocateBuild: error executing locatebuild template: %v", err)
+ log.Errorf("error executing findbuild template: %v", err)
+ http.Error(w, err.Error(), http.StatusInternalServerError)
}
return
}
cl := r.FormValue("cl")
// If no CL value specified in request, display empty CL form
if cl == "" {
- err = locateBuildTemplate.Execute(w, &locateBuildPage{Internal: true})
+ err = findBuildTemplate.Execute(w, &findBuildPage{Internal: true})
if err != nil {
- log.Errorf("HandleLocateBuild: error executing locatebuild template: %v", err)
+ log.Errorf("error executing findbuild template: %v", err)
+ http.Error(w, err.Error(), http.StatusInternalServerError)
}
return
}
@@ -464,10 +381,16 @@
if r.FormValue("internal") == "true" {
internal, gerrit, fallbackGerrit, gob, repo = true, internalGerritInstance, internalFallbackGerritInstance, internalGoBInstance, internalManifestRepo
}
- buildData, didFallback, err := findBuildWithFallback(httpClient, gerrit, fallbackGerrit, gob, repo, cl, internal)
+ httpClient, err := HTTPClient(w, r)
if err != nil {
- log.Errorf("Handlelocatebuild: error retrieving build for CL %s with internal set to %t\n%v", cl, internal, err)
- handleError(w, err, "/locatebuild/")
+ loginURL := GetLoginURL("/findbuild/", false)
+ http.Redirect(w, r, loginURL, http.StatusTemporaryRedirect)
+ return
+ }
+ buildData, didFallback, utilErr := findBuildWithFallback(httpClient, gerrit, fallbackGerrit, gob, repo, cl, internal)
+ if utilErr != nil {
+ log.Errorf("error retrieving build for CL %s with internal set to %t\n%v", cl, internal, utilErr)
+ handleError(w, r, utilErr, "/findbuild/")
return
}
var gerritLink string
@@ -476,15 +399,16 @@
} else {
gerritLink = gerrit + "/q/" + buildData.CLNum
}
- page := &locateBuildPage{
+ page := &findBuildPage{
CL: cl,
CLNum: buildData.CLNum,
BuildNum: buildData.BuildNum,
Internal: internal,
GerritLink: gerritLink,
}
- err = locateBuildTemplate.Execute(w, page)
+ err = findBuildTemplate.Execute(w, page)
if err != nil {
- log.Errorf("HandleLocateBuild: error executing locatebuild template: %v", err)
+ log.Errorf("error executing findbuild template: %v", err)
+ http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
diff --git a/src/cmd/changelog-webapp/main.go b/src/cmd/changelog-webapp/main.go
index 3813591..5d2caa7 100644
--- a/src/cmd/changelog-webapp/main.go
+++ b/src/cmd/changelog-webapp/main.go
@@ -39,11 +39,10 @@
http.Handle("/static/", http.StripPrefix("/static/", http.FileServer(http.Dir(staticBasePath))))
http.HandleFunc("/", controllers.HandleIndex)
http.HandleFunc("/changelog/", controllers.HandleChangelog)
- http.HandleFunc("/locatebuild/", controllers.HandleLocateBuild)
- http.HandleFunc("/login/", func(w http.ResponseWriter, r *http.Request) {
- controllers.HandleLogin(w, r, "/")
- })
+ http.HandleFunc("/findbuild/", controllers.HandleFindBuild)
+ http.HandleFunc("/login/", controllers.HandleLogin)
http.HandleFunc("/oauth2callback/", controllers.HandleCallback)
+ http.HandleFunc("/signout/", controllers.HandleSignOut)
if port == "" {
port = "8081"
diff --git a/src/cmd/changelog-webapp/static/css/base.css b/src/cmd/changelog-webapp/static/css/base.css
index 7d763c7..03a1865 100644
--- a/src/cmd/changelog-webapp/static/css/base.css
+++ b/src/cmd/changelog-webapp/static/css/base.css
@@ -44,7 +44,6 @@
background-color: #4285f4;
display: flex;
height: 50px;
- padding: 0px 25px;
width: 100%;
}
@@ -52,6 +51,17 @@
color: #f8f8f8;
font-size: 24;
font-weight: 350;
+ padding-left: 25px;
+}
+
+a.login,
+a.signout {
+ color: #f8f8f8;
+ float: right;
+ font-size: 16;
+ font-weight: 16px;
+ margin-left: auto;
+ padding-right: 25px;
}
.sidenav {
@@ -62,7 +72,7 @@
padding-top: 30px;
position: fixed;
top: 0;
- width: 150px;
+ width: 155px;
z-index: 1;
}
@@ -86,7 +96,7 @@
}
.main {
- margin: 30px 25px 0px 160px;
+ margin: 30px 25px 0px 155px;
}
.text-content {
diff --git a/src/cmd/changelog-webapp/static/css/changelog.css b/src/cmd/changelog-webapp/static/css/changelog.css
index 5098deb..1301694 100644
--- a/src/cmd/changelog-webapp/static/css/changelog.css
+++ b/src/cmd/changelog-webapp/static/css/changelog.css
@@ -10,10 +10,16 @@
padding: 2px;
}
+.feature-info {
+ font-size: 14;
+ margin: 6px 0px;
+ width: 650px;
+}
+
.changelog-form .text input {
font-size: 14px;
height: 26px;
- margin-bottom: 8px;
+ margin: 8px 2px;
width: 135px;
}
@@ -23,10 +29,7 @@
.changelog-form .text .submit {
width: 59px;
-}
-
-.changelog-form .text label {
- margin: 4px;
+ margin: 0px;
}
.changelog-form .radio .external {
diff --git a/src/cmd/changelog-webapp/static/templates/changelog.html b/src/cmd/changelog-webapp/static/templates/changelog.html
index e5a3c3d..92c2fc0 100644
--- a/src/cmd/changelog-webapp/static/templates/changelog.html
+++ b/src/cmd/changelog-webapp/static/templates/changelog.html
@@ -7,15 +7,17 @@
<body>
<div class="navbar">
<p class="navbar-title">Container Optimized OS</p>
+ <a class="signout" href="/signout/?redirect=/changelog/">Sign Out</a>
</div>
<div class="sidenav">
<a href="/">Home</a>
<a class="active" href="/changelog/">Changelog</a>
- <a href="/locatebuild/">Locate Build</a>
- <a href="/login/">Login</a>
+ <a href="/findbuild/">Find Build</a>
</div>
<div class="main">
<h1>Search Changelog</h1>
+ <p class="feature-info">Retrieve a list of commits between two Container Optimized OS build numbers.</p>
+ <p class="feature-info">Note: Build numbers generated using ChromiumOS infrastructure are not supported.</p>
<form class="changelog-form" action="/changelog">
<div class="text">
<label>From </label>
diff --git a/src/cmd/changelog-webapp/static/templates/error.html b/src/cmd/changelog-webapp/static/templates/error.html
index 0264ba4..e880f95 100644
--- a/src/cmd/changelog-webapp/static/templates/error.html
+++ b/src/cmd/changelog-webapp/static/templates/error.html
@@ -6,6 +6,11 @@
<body>
<div class="navbar">
<p class="navbar-title">Container Optimized OS</p>
+ {{if .SignedIn}}
+ <a class="signout" href="/signout/?redirect={{.ActivePage}}">Sign Out</a>
+ {{else}}
+ <a class="login" href="/login/?redirect={{.ActivePage}}">Login</a>
+ {{end}}
</div>
<div class="sidenav">
{{if (eq .ActivePage "home")}}
@@ -18,17 +23,18 @@
{{else}}
<a href="/changelog/">Changelog</a>
{{end}}
- {{if (eq .ActivePage "/locatebuild/")}}
- <a class="active" href="/locatebuild/">Locate Build</a>
+ {{if (eq .ActivePage "/findbuild/")}}
+ <a class="active" href="/findbuild/">Find Build</a>
{{else}}
- <a href="/locatebuild/">Locate Build</a>
+ <a href="/findbuild/">Find Build</a>
{{end}}
- <a href="/login/">Login</a>
</div>
<div class="main">
- <h1>{{ .Header }}</h1>
- <p>{{ .Body }}</p>
- <a href={{.ActivePage}}>Go back</a>
+ <div class="text-content">
+ <h1>{{ .Header }}</h1>
+ <p>{{ .Body }}</p>
+ <a href={{.ActivePage}}>Go back</a>
+ </div>
</div>
</body>
diff --git a/src/cmd/changelog-webapp/static/templates/locateBuild.html b/src/cmd/changelog-webapp/static/templates/findBuild.html
similarity index 76%
rename from src/cmd/changelog-webapp/static/templates/locateBuild.html
rename to src/cmd/changelog-webapp/static/templates/findBuild.html
index ca09a73..b48cb19 100644
--- a/src/cmd/changelog-webapp/static/templates/locateBuild.html
+++ b/src/cmd/changelog-webapp/static/templates/findBuild.html
@@ -7,16 +7,18 @@
<body>
<div class="navbar">
<p class="navbar-title">Container Optimized OS</p>
+ <a class="signout" href="/signout/?redirect=/findbuild/">Sign Out</a>
</div>
<div class="sidenav">
<a href="/">Home</a>
<a href="/changelog/">Changelog</a>
- <a class="active" href="/locatebuild/">Locate Build</a>
- <a href="/login/">Login</a>
+ <a class="active" href="/findbuild/">Find Build</a>
</div>
<div class="main">
- <h1>Locate Build with CL</h1>
- <form class="changelog-form" action="/locatebuild">
+ <h1>Find Build with CL</h1>
+ <p class="feature-info">Find the first build containing a requested CL.</p>
+ <p class="feature-info">Note: It is possible that a COS build was not created using the build number returned. However, any COS build with build number greater than the returned build number should have the desired change present.</p>
+ <form class="changelog-form" action="/findbuild">
<div class="text">
{{if (ne .CL "")}}
<input type="text" class="cl-input" name="cl" placeholder="CL-Number or Commit-SHA" value={{.CL}} autocomplete="off" required>
diff --git a/src/cmd/changelog-webapp/static/templates/index.html b/src/cmd/changelog-webapp/static/templates/index.html
index 350a422..1aaf48e 100644
--- a/src/cmd/changelog-webapp/static/templates/index.html
+++ b/src/cmd/changelog-webapp/static/templates/index.html
@@ -6,12 +6,16 @@
<body>
<div class="navbar">
<p class="navbar-title">Container Optimized OS</p>
+ {{if .SignedIn}}
+ <a class="signout" href="/signout/">Sign Out</a>
+ {{else}}
+ <a class="login" href="/login/">Login</a>
+ {{end}}
</div>
<div class="sidenav">
<a class="active" href="/">Home</a>
<a href="/changelog/">Changelog</a>
- <a href="/locatebuild/">Locate Build</a>
- <a href="/login/">Login</a>
+ <a href="/findbuild/">Find Build</a>
</div>
<div class="main">
<div class="text-content">
diff --git a/src/cmd/changelog-webapp/static/templates/promptLogin.html b/src/cmd/changelog-webapp/static/templates/promptLogin.html
index 9ca8fb9..cab2f47 100644
--- a/src/cmd/changelog-webapp/static/templates/promptLogin.html
+++ b/src/cmd/changelog-webapp/static/templates/promptLogin.html
@@ -6,6 +6,7 @@
<body>
<div class="navbar">
<p class="navbar-title">Container Optimized OS</p>
+ <a class="login" href="/login/?redirect={{.ActivePage}}">Login</a>
</div>
<div class="sidenav">
<a href="/">Home</a>
@@ -14,18 +15,18 @@
{{else}}
<a href="/changelog/">Changelog</a>
{{end}}
- {{if (eq .ActivePage "/locatebuild/")}}
- <a class="active" href="/locatebuild/">Locate Build</a>
+ {{if (eq .ActivePage "/findbuild/")}}
+ <a class="active" href="/findbuild/">Find Build</a>
{{else}}
- <a href="/locatebuild/">Locate Build</a>
+ <a href="/findbuild/">Find Build</a>
{{end}}
- <a href="/login/">Login</a>
</div>
<div class="main">
<div class="text-content">
<h1>Please sign in to use this feature</h1>
<p>This application requires OAuth authentication to communicate with Google Git repositories. Your account data
- will not be used for any other purposes. To continue, please sign in by clicking <a href="/login/">here</a>.
+ will not be used for any other purposes. To continue, please sign in by clicking
+ <a href="/login/?redirect={{.ActivePage}}">here</a>.
</p>
</div>
</div>
diff --git a/src/cmd/changelog/README.md b/src/cmd/changelog/README.md
index efb241d..3730617 100644
--- a/src/cmd/changelog/README.md
+++ b/src/cmd/changelog/README.md
@@ -45,4 +45,7 @@
## FindCL output
-Prints the first build number that includes the input CL.
\ No newline at end of file
+Prints the first build number that includes the input CL.
+
+## Notes
+* Changelog only supports querying from build numbers from [COS GoB](cos.googlesource.com). It does not support build numbers generated using [ChromiumOS GoB](https://chromium.googlesource.com/).
\ No newline at end of file
diff --git a/src/cmd/changelog/main.go b/src/cmd/changelog/main.go
index 00b0d79..6332e19 100755
--- a/src/cmd/changelog/main.go
+++ b/src/cmd/changelog/main.go
@@ -51,14 +51,6 @@
fallbackRepoPrefix = "mirrors/cros/"
)
-func unwrappedError(err error) error {
- innerErr := err
- for errors.Unwrap(innerErr) != nil {
- innerErr = errors.Unwrap(innerErr)
- }
- return innerErr
-}
-
func getHTTPClient() (*http.Client, error) {
log.Debug("Creating HTTP client")
creds, err := google.FindDefaultCredentials(context.Background(), gerrit.OAuthScope)
@@ -107,7 +99,7 @@
func getBuildForCL(gerrit, fallback, gob, manifestRepo, fallbackPrefix, targetCL string) error {
httpClient, err := getHTTPClient()
if err != nil {
- return fmt.Errorf("Error creating http client: %v", err)
+ return fmt.Errorf("error creating http client: %v", err)
}
req := &findbuild.BuildRequest{
HTTPClient: httpClient,
@@ -117,8 +109,8 @@
RepoPrefix: "",
CL: targetCL,
}
- buildData, err := findbuild.FindBuild(req)
- if unwrappedError(err) == findbuild.ErrorCLNotFound {
+ buildData, clErr := findbuild.FindBuild(req)
+ if clErr != nil && clErr.HTTPCode() == "404" {
log.Debugf("Query failed on Gerrit url %s and Gitiles url %s, retrying with fallback urls", externalGerritURL, externalGoBURL)
fallbackReq := &findbuild.BuildRequest{
HTTPClient: httpClient,
@@ -128,10 +120,10 @@
RepoPrefix: fallbackPrefix,
CL: targetCL,
}
- buildData, err = findbuild.FindBuild(fallbackReq)
+ buildData, clErr = findbuild.FindBuild(fallbackReq)
}
- if err != nil {
- return err
+ if clErr != nil {
+ return clErr
}
log.Infof("Build: %s", buildData.BuildNum)
return nil
@@ -211,7 +203,7 @@
target := c.Args().Get(1)
return generateChangelog(source, target, gobURL, manifestRepo)
default:
- return fmt.Errorf("Please specify either \"findbuild\" or \"changelog\" mode")
+ return fmt.Errorf("please specify either \"findbuild\" or \"changelog\" mode")
}
},
}
diff --git a/src/cmd/changelog/main_test.py b/src/cmd/changelog/main_test.py
index 71e5546..d881eb7 100644
--- a/src/cmd/changelog/main_test.py
+++ b/src/cmd/changelog/main_test.py
@@ -114,7 +114,7 @@
def test_large_run(self):
source = "15055.0.0"
- target = "15000.0.0"
+ target = "15030.0.0"
instance = "cos.googlesource.com"
repo = "cos/manifest-snapshots"
delete_logs(source, target)
@@ -122,7 +122,7 @@
assert process.returncode == 0
assert check_file_exists(source, target)
assert check_file_exists(target, source)
- assert check_changelog_schema(source, target)
+ assert check_empty_json_file(source, target)
assert check_changelog_schema(target, source)
def test_with_invalid_source(self):
diff --git a/src/pkg/changelog/changelog.go b/src/pkg/changelog/changelog.go
index 098138a..9db2099 100644
--- a/src/pkg/changelog/changelog.go
+++ b/src/pkg/changelog/changelog.go
@@ -25,13 +25,12 @@
// URL. A request is sent on a seperate thread for each repository, asking for a list
// of commits that occurred between the source committish and the target committish.
// Finally, the resulting git.Commit objects are converted to Commit objects, and
-// consolidated into a mapping of repositoryName -> []*Commit.
+// consolidated into a mapping of repository path -> []*Commit.
package changelog
import (
"errors"
- "fmt"
"net/http"
"strings"
@@ -43,6 +42,8 @@
)
type repo struct {
+ Repo string
+ Path string
// The Git on Borg instance to query from.
InstanceURL string
// A value that points to the last commit for a build on a given repo.
@@ -56,53 +57,85 @@
}
type commitsResult struct {
- RepoURL string
Commits []*Commit
+ Repo string
+ Path string
HasMoreCommits bool
- Err error
+ Err utils.ChangelogError
}
type additionsResult struct {
Additions map[string]*RepoLog
- Err error
+ Err utils.ChangelogError
}
-func gerritClient(httpClient *http.Client, remoteURL string) (gitilesProto.GitilesClient, error) {
- log.Debugf("Creating Gerrit client for remote url %s\n", remoteURL)
+// RepoLog contains a changelist for a particular repository
+type RepoLog struct {
+ Commits []*Commit
+ Repo string
+ SourceSHA string
+ TargetSHA string
+ HasMoreCommits bool
+}
+
+// Creates a unique identifier for a repo + branch pairing.
+// Path is used instead of dest-branch because some manifest files do not
+// indicate a dest-branch for a project.
+// Path itself is not sufficient to guarantee uniqueness, since some repos
+// share the same path.
+// ex. mirrors/cros/chromiumos/repohooks vs cos/repohooks
+func repoID(name, path string) string {
+ return name + path
+}
+
+// limitPageSize will restrict a request page size to min of pageSize (which grows exponentially)
+// or remaining request size
+func limitPageSize(pageSize, requestedSize int) int {
+ if requestedSize == -1 || pageSize <= requestedSize {
+ return pageSize
+ }
+ return requestedSize
+}
+
+func gitilesClient(httpClient *http.Client, remoteURL string) (gitilesProto.GitilesClient, utils.ChangelogError) {
+ log.Debugf("Creating Gitiles client for remote url %s\n", remoteURL)
cl, err := gitilesApi.NewRESTClient(httpClient, remoteURL, true)
if err != nil {
- return nil, errors.New("changelog: Failed to establish client to remote url: " + remoteURL)
+ log.Errorf("gitilesClient: failed to create client for remote url %s", remoteURL)
+ return nil, utils.InternalError
}
return cl, nil
}
-func createGerritClients(clients map[string]gitilesProto.GitilesClient, httpClient *http.Client, repoMap map[string]*repo) error {
+func createGitilesClients(clients map[string]gitilesProto.GitilesClient, httpClient *http.Client, repoMap map[string]*repo) utils.ChangelogError {
log.Debug("Creating additional Gerrit clients for manifest file if not already created")
for _, repoData := range repoMap {
remoteURL := repoData.InstanceURL
if _, ok := clients[remoteURL]; ok {
continue
}
- client, err := gerritClient(httpClient, remoteURL)
+ client, err := gitilesClient(httpClient, remoteURL)
if err != nil {
- return fmt.Errorf("createClients: error creating client mapping:\n%w", err)
+ return err
}
clients[remoteURL] = client
}
return nil
}
-// repoMap generates a mapping of repo name to instance URL and committish.
+// repoMap generates a mapping of repository ID to instance URL and committish.
// This eliminates the need to track remote names and allows lookup
// of source committish when generating changelog.
func repoMap(manifest string) (map[string]*repo, error) {
log.Debug("Mapping repository to instance URL and committish")
if manifest == "" {
- return nil, fmt.Errorf("repoMap: manifest data is empty")
+ log.Error("repoMap: manifest file is empty")
+ return nil, errors.New("manifest file is empty")
}
doc := etree.NewDocument()
if err := doc.ReadFromString(manifest); err != nil {
- return nil, fmt.Errorf("repoMap: error parsing manifest xml:\n%w", err)
+ log.Debug("repoMap: error parsing manifest xml:\n%w", err)
+ return nil, errors.New("could not parse XML for manifest file associated with build")
}
root := doc.SelectElement("manifest")
@@ -123,7 +156,10 @@
}
repos := make(map[string]*repo)
for _, project := range root.SelectElements("project") {
- repos[project.SelectAttr("name").Value] = &repo{
+ name, path := project.SelectAttr("name").Value, project.SelectAttr("path").Value
+ repos[repoID(name, path)] = &repo{
+ Repo: name,
+ Path: path,
InstanceURL: remoteMap[project.SelectAttrValue("remote", "")],
Committish: project.SelectAttr("revision").Value,
}
@@ -132,35 +168,41 @@
}
// mappedManifest retrieves a Manifest file from GoB and unmarshals XML.
-func mappedManifest(client gitilesProto.GitilesClient, repo string, buildNum string) (map[string]*repo, error) {
+// Returns a mapping of repository ID to repository data.
+func mappedManifest(client gitilesProto.GitilesClient, repo string, buildNum string) (map[string]*repo, utils.ChangelogError) {
log.Debugf("Retrieving manifest file for build %s\n", buildNum)
response, err := utils.DownloadManifest(client, repo, buildNum)
if err != nil {
- return nil, fmt.Errorf("mappedManifest: error downloading manifest file from repo %s:\n%w",
- repo, err)
+ log.Errorf("mappedManifest: error downloading manifest file from repo %s for build %s:\n%v", repo, buildNum, err)
+ httpCode := utils.GitilesErrCode(err)
+ return nil, utils.FromChangelogError(httpCode, buildNum)
}
mappedManifest, err := repoMap(response.Contents)
if err != nil {
- return nil, fmt.Errorf("mappedManifest: error parsing manifest contents from repo %s:\n%w",
- repo, err)
+ log.Errorf("mappedManifest: error retrieving mapped manifest file from repo %s for build %s:\n%v", repo, buildNum, err)
+ httpCode := utils.GitilesErrCode(err)
+ return nil, utils.FromChangelogError(httpCode, buildNum)
}
return mappedManifest, nil
}
-// commits retrieves a parsed list of commits between an ancestor and a committish
-func commits(client gitilesProto.GitilesClient, repo string, committish string, ancestor string, querySize int, outputChan chan commitsResult) {
+// commits get all commits that occur between committish and ancestor for a specific repo.
+func commits(client gitilesProto.GitilesClient, path string, repo string, committish string, ancestor string, querySize int, outputChan chan commitsResult) {
log.Debugf("Fetching changelog for repo: %s on committish %s\n", repo, committish)
commits, hasMoreCommits, err := utils.Commits(client, repo, committish, ancestor, querySize)
if err != nil {
- outputChan <- commitsResult{Err: err}
+ outputChan <- commitsResult{Err: utils.InternalError}
}
parsedCommits, err := ParseGitCommitLog(commits)
if err != nil {
- outputChan <- commitsResult{Err: err}
+ log.Errorf("commits: Error parsing Gitiles commits response\n%v", err)
+ outputChan <- commitsResult{Err: utils.InternalError}
+ return
}
outputChan <- commitsResult{
- RepoURL: repo,
Commits: parsedCommits,
+ Path: path,
+ Repo: repo,
HasMoreCommits: hasMoreCommits,
}
}
@@ -171,15 +213,15 @@
log.Debug("Retrieving commit additions")
repoCommits := make(map[string]*RepoLog)
commitsChan := make(chan commitsResult, len(targetRepos))
- for repoURL, targetRepoInfo := range targetRepos {
+ for repoID, targetRepoInfo := range targetRepos {
cl := clients[targetRepoInfo.InstanceURL]
// If the source Manifest file does not contain a target repo,
// count every commit since target repo creation as an addition
ancestorCommittish := ""
- if sourceRepoInfo, ok := sourceRepos[repoURL]; ok {
+ if sourceRepoInfo, ok := sourceRepos[repoID]; ok {
ancestorCommittish = sourceRepoInfo.Committish
}
- go commits(cl, repoURL, targetRepoInfo.Committish, ancestorCommittish, querySize, commitsChan)
+ go commits(cl, targetRepoInfo.Path, targetRepoInfo.Repo, targetRepoInfo.Committish, ancestorCommittish, querySize, commitsChan)
}
for i := 0; i < len(targetRepos); i++ {
res := <-commitsChan
@@ -187,16 +229,17 @@
outputChan <- additionsResult{Err: res.Err}
return
}
- sourceSHA := ""
- if sha, ok := sourceRepos[res.RepoURL]; ok {
- sourceSHA = sha.Committish
+ var sourceSHA string
+ if sourceData, ok := sourceRepos[repoID(res.Repo, res.Path)]; ok {
+ sourceSHA = sourceData.Committish
}
if len(res.Commits) > 0 {
- repoCommits[res.RepoURL] = &RepoLog{
+ repoCommits[res.Path] = &RepoLog{
Commits: res.Commits,
HasMoreCommits: res.HasMoreCommits,
+ Repo: res.Repo,
SourceSHA: sourceSHA,
- TargetSHA: targetRepos[res.RepoURL].Committish,
+ TargetSHA: targetRepos[repoID(res.Repo, res.Path)].Committish,
}
}
}
@@ -227,9 +270,10 @@
//
// The second changelog contains all commits that are present in the source build
// but not present in the target build
-func Changelog(httpClient *http.Client, sourceBuildNum string, targetBuildNum string, host string, repo string, querySize int) (map[string]*RepoLog, map[string]*RepoLog, error) {
+func Changelog(httpClient *http.Client, sourceBuildNum string, targetBuildNum string, host string, repo string, querySize int) (map[string]*RepoLog, map[string]*RepoLog, utils.ChangelogError) {
if httpClient == nil {
- return nil, nil, errors.New("Changelog: httpClient should not be nil")
+ log.Error("httpClient is nil")
+ return nil, nil, utils.InternalError
}
log.Infof("Retrieving changelog between %s and %s\n", sourceBuildNum, targetBuildNum)
@@ -237,29 +281,27 @@
// Since the manifest file is always in the cos instance, add cos client
// so that client knows what URL to use
- manifestClient, err := gerritClient(httpClient, host)
+ manifestClient, err := gitilesClient(httpClient, host)
if err != nil {
- return nil, nil, fmt.Errorf("Changelog: error creating client for GoB instance: %s:\n%w", host, err)
+ return nil, nil, err
}
sourceRepos, err := mappedManifest(manifestClient, repo, sourceBuildNum)
if err != nil {
- return nil, nil, fmt.Errorf("Changelog: error retrieving mapped manifest for source build number: %s using manifest repository: %s:\n%w",
- sourceBuildNum, repo, err)
+ return nil, nil, err
}
targetRepos, err := mappedManifest(manifestClient, repo, targetBuildNum)
if err != nil {
- return nil, nil, fmt.Errorf("Changelog: error retrieving mapped manifest for target build number: %s using manifest repository: %s:\n%w",
- targetBuildNum, repo, err)
+ return nil, nil, err
}
clients[host] = manifestClient
- err = createGerritClients(clients, httpClient, sourceRepos)
+ err = createGitilesClients(clients, httpClient, sourceRepos)
if err != nil {
- return nil, nil, fmt.Errorf("Changelog: error creating source clients:\n%w", err)
+ return nil, nil, err
}
- err = createGerritClients(clients, httpClient, targetRepos)
+ err = createGitilesClients(clients, httpClient, targetRepos)
if err != nil {
- return nil, nil, fmt.Errorf("Changelog: error creating target clients:\n%w", err)
+ return nil, nil, err
}
addChan := make(chan additionsResult, 1)
@@ -268,11 +310,11 @@
go additions(clients, targetRepos, sourceRepos, querySize, missChan)
missRes := <-missChan
if missRes.Err != nil {
- return nil, nil, fmt.Errorf("Changelog: failure when retrieving missed commits:\n%w", missRes.Err)
+ return nil, nil, err
}
addRes := <-addChan
if addRes.Err != nil {
- return nil, nil, fmt.Errorf("Changelog: failure when retrieving commit additions:\n%w", addRes.Err)
+ return nil, nil, err
}
return addRes.Additions, missRes.Additions, nil
diff --git a/src/pkg/changelog/changelog_test.go b/src/pkg/changelog/changelog_test.go
index c7ebdcd..c379392 100644
--- a/src/pkg/changelog/changelog_test.go
+++ b/src/pkg/changelog/changelog_test.go
@@ -51,17 +51,17 @@
return true
}
-func repoListInLog(log map[string]*RepoLog, check []string) bool {
+func repoListInLog(log map[string]*RepoLog, check []string) error {
for _, check := range check {
if log, ok := log[check]; !ok || len(log.Commits) == 0 {
- return false
+ return fmt.Errorf("Repo path %s not in log", check)
}
}
- return true
+ return nil
}
func TestChangelog(t *testing.T) {
- httpClient, err := getHTTPClient()
+ httpClient, _ := getHTTPClient()
// Test invalid source
additions, removals, err := Changelog(httpClient, "15", "15043.0.0", cosInstance, defaultManifestRepo, -1)
@@ -71,6 +71,8 @@
t.Errorf("changelog failed, expected nil removals, got %v", removals)
} else if err == nil {
t.Errorf("changelog failed, expected error, got nil")
+ } else if err.HTTPCode() != "404" {
+ t.Errorf("changelog failed, expected error code 404, got error code %s", err.HTTPCode())
}
// Test invalid target
@@ -81,6 +83,8 @@
t.Errorf("changelog failed, expected nil removals, got %v", removals)
} else if err == nil {
t.Errorf("changelog failed, expected error, got nil")
+ } else if err.HTTPCode() != "404" {
+ t.Errorf("changelog failed, expected error code 404, got error code %s", err.HTTPCode())
}
// Test invalid instance
@@ -91,6 +95,8 @@
t.Errorf("changelog failed, expected nil removals, got %v", removals)
} else if err == nil {
t.Errorf("changelog failed, expected error, got nil")
+ } else if err.HTTPCode() != "500" {
+ t.Errorf("changelog failed, expected error code 500, got error code %s", err.HTTPCode())
}
// Test invalid manifest repo
@@ -101,6 +107,8 @@
t.Errorf("changelog failed, expected nil removals, got %v", removals)
} else if err == nil {
t.Errorf("changelog failed, expected error, got nil")
+ } else if err.HTTPCode() != "404" {
+ t.Errorf("changelog failed, expected error code 404, got error code %s", err.HTTPCode())
}
// Test build number higher than latest release
@@ -111,6 +119,8 @@
t.Errorf("changelog failed, expected nil removals, got %v", removals)
} else if err == nil {
t.Errorf("changelog failed, expected error, got nil")
+ } else if err.HTTPCode() != "404" {
+ t.Errorf("changelog failed, expected error code 404, got error code %s", err.HTTPCode())
}
// Test manifest with remote urls specified and no default URL
@@ -246,11 +256,11 @@
} else if len(additions) != 1 {
t.Errorf("changelog failed, expected only 1 repo in additions, got %v", additions)
}
- boardOverlayLog := additions["cos/overlays/board-overlays"]
+ boardOverlayLog := additions["src/overlays"]
if boardOverlayLog == nil {
- t.Errorf("changelog failed, expected cos/overlays/board-overlays in changelog, got nil")
+ t.Errorf("Changelog failed, expected src/overlays in changelog, got nil")
} else if changes := boardOverlayLog.Commits; len(changes) != 108 {
- t.Errorf("changelog failed, expected 108 changes for \"cos/overlays/board-overlays\", got %d", len(changes))
+ t.Errorf("Changelog failed, expected 108 changes for \"src/overlays\", got %d", len(changes))
} else if !commitsMatch(boardOverlayLog.Commits, expectedCommits) {
t.Errorf("changelog failed, Changelog output does not match expected commits or is not sorted")
} else if boardOverlayLog.SourceSHA != "612ca5ef5455534127d008e08c65aa29a2fd97a5" {
@@ -261,37 +271,35 @@
// Test build numbers further apart from each other with multiple repo differences
// Also ensures that removals are correctly populated
- source = "15020.0.0"
+ source = "15030.0.0"
target = "15056.0.0"
additionRepos := []string{
- "mirrors/cros/chromiumos/platform/crosutils",
- "cos/manifest",
- "mirrors/cros/chromiumos/platform/vboot_reference",
- "mirrors/cros/chromiumos/platform/dev-util",
- "mirrors/cros/chromiumos/platform/crostestutils",
- "mirrors/cros/chromiumos/infra/proto",
- "mirrors/cros/chromiumos/third_party/toolchain-utils",
- "mirrors/cros/chromiumos/third_party/coreboot",
- "cos/overlays/board-overlays",
- "mirrors/cros/chromiumos/platform2",
- "mirrors/cros/chromiumos/overlays/eclass-overlay",
- "mirrors/cros/chromiumos/chromite",
- "mirrors/cros/chromiumos/third_party/autotest",
- "mirrors/cros/chromiumos/overlays/chromiumos-overlay",
- "third_party/kernel",
- "mirrors/cros/chromium/tools/depot_tools",
- "mirrors/cros/chromiumos/repohooks",
- "mirrors/cros/chromiumos/overlays/portage-stable",
+ "src/scripts",
+ "src/platform/vboot_reference",
+ "src/platform/dev",
+ "chromite",
+ "src/third_party/autotest/files",
+ "src/third_party/eclass-overlay",
+ "src/third_party/toolchain-utils",
+ "src/platform/crostestutils",
+ "src/third_party/coreboot",
+ "src/third_party/kernel/v5.4",
+ "src/overlays",
+ "src/chromium/depot_tools",
+ "src/third_party/portage-stable",
+ "chromite/infra/proto",
+ "manifest",
+ "src/platform2",
+ "src/third_party/chromiumos-overlay",
}
additions, removals, err = Changelog(httpClient, source, target, cosInstance, defaultManifestRepo, -1)
if err != nil {
t.Errorf("changelog failed, expected no error, got %v", err)
}
- kernelLog := additions["third_party/kernel"]
- if len(removals) != 1 || kernelLog == nil {
- t.Errorf("changelog failed, expected miss list containing only \"third_party/kernel\", got %v", removals)
- } else if !repoListInLog(additions, additionRepos) {
- t.Errorf("changelog failed, additions repo output does not match expected repos %v", additionRepos)
+ if len(removals) != 0 {
+ t.Errorf("Changelog failed, expected empty removals, got %v", removals)
+ } else if err := repoListInLog(additions, additionRepos); err != nil {
+ t.Errorf("Changelog failed, additions repo output does not match expected repos: %v", err)
}
// Test changelog returns correct output when given a querySize instead of -1
@@ -304,18 +312,18 @@
} else if additions == nil {
t.Errorf("changelog failed, non-empty expected additions, got nil")
} else if removals == nil {
- t.Errorf("changelog failed, non-empty expected removals, got nil")
- } else if _, ok := additions["third_party/kernel"]; !ok {
- t.Errorf("changelog failed, expected repo: third_party/kernel in additions")
+ t.Errorf("Changelog failed, non-empty expected removals, got nil")
+ } else if _, ok := additions["src/third_party/kernel/v5.4"]; !ok {
+ t.Errorf("Changelog failed, expected repo: src/third_party/kernel/v4.19 in additions")
}
for repoName, repoLog := range additions {
if repoLog.Commits == nil || len(repoLog.Commits) == 0 {
t.Errorf("changelog failed, expected non-empty additions commits, got nil or empty commits")
}
if len(repoLog.Commits) > querySize {
- t.Errorf("changelog failed, expected %d commits for repo: %s, got: %d", querySize, repoName, len(repoLog.Commits))
- } else if repoName == "third_party/kernel" && !repoLog.HasMoreCommits {
- t.Errorf("changelog failed, expected HasMoreCommits = True for repo: third_party/kernel, got False")
+ t.Errorf("Changelog failed, expected %d commits for repo: %s, got: %d", querySize, repoName, len(repoLog.Commits))
+ } else if repoName == "src/third_party/kernel/v5.4" && !repoLog.HasMoreCommits {
+ t.Errorf("Changelog failed, expected HasMoreCommits = True for repo: src/third_party/kernel/v5.4, got False")
} else if repoLog.HasMoreCommits && len(repoLog.Commits) < querySize {
t.Errorf("changelog failed, expected HasMoreCommits = False for repo: %s with %d commits, got True", repoName, len(repoLog.Commits))
}
@@ -328,47 +336,61 @@
if err != nil {
t.Errorf("changelog failed, expected no error, got %v", err)
} else if len(removals) != 0 {
- t.Errorf("changelog failed, expected empty removals, got %v", removals)
- } else if _, ok := additions["cos/cobble"]; !ok {
- t.Errorf("changelog failed, expected repo: third_party/kernel in additions")
+ t.Errorf("Changelog failed, expected empty removals, got %v", removals)
+ } else if _, ok := additions["src/platform/cobble"]; !ok {
+ t.Errorf("Changelog failed, expected repo: src/third_party/kernel/v4.19 in additions")
}
for repoName, repoLog := range additions {
if repoLog.Commits == nil || len(repoLog.Commits) == 0 {
- t.Errorf("changelog failed, expected non-empty additions commits, got nil or empty commits")
- } else if repoName == "cos/cobble" {
+ t.Errorf("Changelog failed, expected non-empty additions commits, got nil or empty commits")
+ } else if repoName == "src/platform/cobble" {
if repoLog.HasMoreCommits {
- t.Errorf("changelog failed, expected hasMoreCommits = false for repo: cos/cobble, got true")
+ t.Errorf("Changelog failed, expected hasMoreCommits = false for repo: src/platform/cobble, got true")
} else if repoLog.SourceSHA != "" {
- t.Errorf("changelog failed, expected empty SourceSHA for cos/cobble, got %s", repoLog.SourceSHA)
+ t.Errorf("Changelog failed, expected empty SourceSHA for src/platform/cobble, got %s", repoLog.SourceSHA)
} else if repoLog.TargetSHA != "4ab43f1f86b7099b8ad75cf9615ea1fa155bbd7d" {
- t.Errorf("changelog failed, expected TargetSHA: \"4ab43f1f86b7099b8ad75cf9615ea1fa155bbd7d\" for cos/cobble, got %s", repoLog.TargetSHA)
+ t.Errorf("Changelog failed, expected TargetSHA: \"4ab43f1f86b7099b8ad75cf9615ea1fa155bbd7d\" for src/platform/cobble, got %s", repoLog.TargetSHA)
}
}
}
- // Test repository added between builds
+
+ // Test changelog handles new repository addition
source = "12871.1179.0"
target = "12871.1177.0"
additions, removals, err = Changelog(httpClient, source, target, cosInstance, defaultManifestRepo, querySize)
if err != nil {
t.Errorf("changelog failed, expected no error, got %v", err)
} else if len(additions) != 0 {
- t.Errorf("changelog failed, expected empty additions, got %v", additions)
- } else if _, ok := removals["cos/cobble"]; !ok {
- t.Errorf("changelog failed, expected repo: third_party/kernel in additions")
+ t.Errorf("Changelog failed, expected empty additions, got %v", additions)
+ } else if _, ok := removals["src/platform/cobble"]; !ok {
+ t.Errorf("Changelog failed, expected repo: src/third_party/kernel/v4.19 in additions")
}
for repoName, repoLog := range removals {
if repoLog.Commits == nil || len(repoLog.Commits) == 0 {
- t.Errorf("changelog failed, expected non-empty additions commits, got nil or empty commits")
- } else if repoName == "cos/cobble" {
+ t.Errorf("Changelog failed, expected non-empty additions commits, got nil or empty commits")
+ } else if repoName == "src/platform/cobble" {
if repoLog.HasMoreCommits {
- t.Errorf("changelog failed, expected hasMoreCommits = false for repo: cos/cobble, got true")
+ t.Errorf("Changelog failed, expected hasMoreCommits = false for repo: src/platform/cobble, got true")
} else if repoLog.SourceSHA != "" {
- t.Errorf("changelog failed, expected empty SourceSHA for cos/cobble, got %s", repoLog.SourceSHA)
+ t.Errorf("Changelog failed, expected empty SourceSHA for src/platform/cobble, got %s", repoLog.SourceSHA)
} else if repoLog.TargetSHA != "4ab43f1f86b7099b8ad75cf9615ea1fa155bbd7d" {
- t.Errorf("changelog failed, expected TargetSHA: \"4ab43f1f86b7099b8ad75cf9615ea1fa155bbd7d\" for cos/cobble, got %s", repoLog.TargetSHA)
+ t.Errorf("Changelog failed, expected TargetSHA: \"4ab43f1f86b7099b8ad75cf9615ea1fa155bbd7d\" for src/platform/cobble, got %s", repoLog.TargetSHA)
}
}
}
+
+ // Test with different release branches
+ source = "13310.1035.0"
+ target = "15000.0.0"
+ additions, removals, err = Changelog(httpClient, source, target, cosInstance, defaultManifestRepo, querySize)
+ if err != nil {
+ t.Errorf("Changelog failed, expected no error, got %v", err)
+ } else if len(additions) == 0 {
+ t.Errorf("Changelog failed, expected non-empty additions, got %v", additions)
+ } else if len(removals) == 0 {
+ t.Errorf("Changelog failed, expected non-empty removals, got %v", removals)
+ }
+
// Test empty repository
source = "0.0.0"
target = "2.0.0"
@@ -379,5 +401,7 @@
t.Errorf("changelog failed, expected nil removals, got %v", removals)
} else if err == nil {
t.Errorf("changelog failed, expected error, got nil")
+ } else if err.HTTPCode() != "500" {
+ t.Errorf("changelog failed, expected error code 500, got error code %s", err.HTTPCode())
}
}
diff --git a/src/pkg/changelog/gitcommit.go b/src/pkg/changelog/gitcommit.go
index 39d49f4..692ff79 100755
--- a/src/pkg/changelog/gitcommit.go
+++ b/src/pkg/changelog/gitcommit.go
@@ -27,14 +27,6 @@
releaseNoteLinePrefix string = "RELEASE_NOTE="
)
-// RepoLog contains a changelist for a particular repository
-type RepoLog struct {
- Commits []*Commit
- SourceSHA string
- TargetSHA string
- HasMoreCommits bool
-}
-
// Commit is a simplified struct of git.Commit
// Useful for interfaces
type Commit struct {
@@ -122,7 +114,7 @@
// Commit object with processed fields
func parseGitCommit(commit *git.Commit) (*Commit, error) {
if commit == nil {
- return nil, errors.New("ParseCommit: Input should not be nil")
+ return nil, errors.New("parseCommit: Input should not be nil")
}
return &Commit{
SHA: commit.Id,
@@ -139,13 +131,13 @@
// into a slice of Commit objects with processed fields
func ParseGitCommitLog(commits []*git.Commit) ([]*Commit, error) {
if commits == nil {
- return nil, errors.New("ParseCommitLog: Input should not be nil")
+ return nil, errors.New("parseCommitLog: Input should not be nil")
}
output := make([]*Commit, len(commits))
for i, commit := range commits {
parsedCommit, err := parseGitCommit(commit)
if err != nil {
- return nil, errors.New("ParseCommitLog: Input slice contains nil pointer")
+ return nil, errors.New("parseCommitLog: Input slice contains nil pointer")
}
output[i] = parsedCommit
}
diff --git a/src/pkg/findbuild/findbuild.go b/src/pkg/findbuild/findbuild.go
index 11814e3..4f32607 100644
--- a/src/pkg/findbuild/findbuild.go
+++ b/src/pkg/findbuild/findbuild.go
@@ -35,7 +35,6 @@
import (
"context"
- "errors"
"fmt"
"net/http"
"regexp"
@@ -76,21 +75,6 @@
defaultRelease: "master",
},
}
-
- // ErrorCLNotLanded indicates no build contains the requested CL
- ErrorCLNotLanded = errors.New("No build found containing CL")
- // ErrorCLNotFound indicates that the CL could not be found in the provided
- // Gerrit instance
- ErrorCLNotFound = errors.New("CL not found in provided Gerrit instance")
- // ErrorIDNotUnique indicates that a provided CL identifier maps to multiple
- // CLs. This could happen if a Change-ID is provided instead of a CL number or
- // commit SHA, since CLs that are cherry-picked are grouped together by
- // Change-ID.
- ErrorIDNotUnique = errors.New("Provided change identifier does not map to a unique commit")
- // ErrorNoManifestBranchMatch indicates that the target CL was for a change
- // in a repository + branch that is not used in any builds within the CL's
- // submission time frame.
- ErrorNoManifestBranchMatch = errors.New("No manifest files were found containing a matching repository and branch as the target CL")
)
// BuildRequest is the input struct for the FindBuild function
@@ -127,7 +111,7 @@
}
type clData struct {
- CLNum int
+ CLNum string
Project string
Release string
Branch string
@@ -149,40 +133,49 @@
Err error
}
-// clList retrieves the list of CLs matching a query from Gerrit
-func clList(client *gerrit.Client, clID string) ([]*gerrit.Change, error) {
+// queryCL retrieves the list of CLs matching a query from Gerrit
+func queryCL(client *gerrit.Client, clID string) (*gerrit.Change, utils.ChangelogError) {
log.Debug("Retrieving CL List from Gerrit")
queryOptions := gerrit.ChangeQueryParams{
Query: clID,
- N: 2,
+ N: 1,
Options: []string{"CURRENT_REVISION"},
}
ctx, cancel := context.WithTimeout(context.Background(), requestMaxAge)
defer cancel()
clList, _, err := client.ChangeQuery(ctx, queryOptions)
- return clList, err
-}
-
-func getCLData(client *gerrit.Client, clID string, manifestPrefix string) (*clData, error) {
- log.Debugf("Retrieving CL data from Gerrit for changeID: %s", clID)
- clList, err := clList(client, clID)
if err != nil {
- return nil, fmt.Errorf("getCLData: Error retrieving change:\n%w", err)
+ log.Errorf("queryCL: Error retrieving change for input %s:\n%v", clID, err)
+ httpCode := utils.GerritErrCode(err)
+ return nil, utils.FromFindBuildError(httpCode, clID)
}
- // A user provided changeID should only map to one CL
if len(clList) == 0 {
- return nil, ErrorCLNotFound
- } else if len(clList) > 1 {
- return nil, ErrorIDNotUnique
+ log.Errorf("queryCL: CL with identifier %s not found", clID)
+ return nil, utils.FromFindBuildError("404", clID)
}
change := clList[0]
+ if change.ChangeID == clID {
+ log.Debugf("Provided CL identifier %s is a Change-ID, should be CL num or commit SHA", clID)
+ return nil, utils.FromFindBuildError("400", clID)
+ }
if change.Submitted == "" {
- return nil, fmt.Errorf("No submitted CL found")
+ log.Debugf("Provided CL identifier %s maps to an unsubmitted CL", clID)
+ return nil, utils.FromFindBuildError("406", clID)
+ }
+ return change, nil
+}
+
+func getCLData(client *gerrit.Client, clID string, manifestPrefix string) (*clData, utils.ChangelogError) {
+ log.Debugf("Retrieving CL data from Gerrit for changeID: %s", clID)
+ change, err := queryCL(client, clID)
+ if err != nil {
+ return nil, err
}
log.Debugf("Target CL found with SHA %s on repo %s, branch %s", change.CurrentRevision, change.Project, change.Branch)
- var parsedTime time.Time
- if parsedTime, err = time.Parse("2006-01-02 15:04:05.000000000", change.Submitted); err != nil {
- return nil, fmt.Errorf("getTargetData: Error parsing CL submit time:\n%w", err)
+ parsedTime, parseErr := time.Parse("2006-01-02 15:04:05.000000000", change.Submitted)
+ if parseErr != nil {
+ log.Errorf("getTargetData: Error parsing submission time %s for CL %d:\n%v", change.Submitted, change.ChangeNumber, err)
+ return nil, utils.InternalError
}
// If a repository has non-conventional branch names, need to convert the
// repository branch name to a release branch name
@@ -195,7 +188,7 @@
}
}
return &clData{
- CLNum: change.ChangeNumber,
+ CLNum: strconv.Itoa(change.ChangeNumber),
Project: manifestPrefix + change.Project,
Release: release,
Branch: change.Branch,
@@ -207,11 +200,12 @@
// candidateManifestCommits returns a list of commits to the manifest-snapshot
// repo that were committed within a time range from the target commit time, in
// reverse chronological order.
-func candidateManifestCommits(client gitilesProto.GitilesClient, manifestRepo string, targetData *clData) ([]*git.Commit, error) {
+func candidateManifestCommits(client gitilesProto.GitilesClient, manifestRepo string, targetData *clData) ([]*git.Commit, utils.ChangelogError) {
log.Debugf("Retrieving all manifest snapshots committed within %d days of CL submission", endBuildTime)
allManifests, _, err := utils.Commits(client, manifestRepo, "refs/heads/"+targetData.Release, "", -1)
if err != nil {
- return nil, err
+ httpCode := utils.GitilesErrCode(err)
+ return nil, utils.FromFindBuildError(httpCode, targetData.CLNum)
}
// Find latest commit that occurs before the target commit time.
// allManifests is in reverse chronological order.
@@ -219,7 +213,8 @@
for left < right {
mid := (left + right) / 2
if allManifests[mid].Committer == nil {
- return nil, fmt.Errorf("Manifest %s has no committer", allManifests[mid].Id)
+ log.Errorf("manifest %s has no committer", allManifests[mid].Id)
+ return nil, utils.InternalError
}
currDate := allManifests[mid].Committer.Time.AsTime()
if currDate.Before(targetData.Time) {
@@ -236,7 +231,8 @@
for left < right {
mid := (left+right)/2 + 1
if allManifests[mid].Committer == nil {
- return nil, fmt.Errorf("Manifest %s has no committer", allManifests[mid].Id)
+ log.Errorf("manifest %s has no committer", allManifests[mid].Id)
+ return nil, utils.InternalError
}
currDate := allManifests[mid].Committer.Time.AsTime()
if currDate.After(endDate) {
@@ -246,6 +242,9 @@
}
}
latestIdx := right
+ if allManifests[earliestIdx].Committer.Time.AsTime().After(targetData.Time) {
+ return nil, utils.FromFindBuildError("404", targetData.CLNum)
+ }
return allManifests[latestIdx : earliestIdx+1], nil
}
@@ -258,7 +257,12 @@
}
ctx, cancel := context.WithTimeout(context.Background(), requestMaxAge)
defer cancel()
- return client.Refs(ctx, request)
+ res, err := client.Refs(ctx, request)
+ if err != nil {
+ log.Errorf("error retrieving tags:\n%v", err)
+ return nil, err
+ }
+ return res, nil
}
// candidateBuildNums returns a list of build numbers from a list of possible
@@ -267,14 +271,16 @@
// could be a candidate. It then retrieves a mapping of build number -> commit SHA,
// for all commits in the manifest repo, and compares it with the candidate
// list to create a list of build numbers.
-func candidateBuildNums(client gitilesProto.GitilesClient, manifestRepo string, targetData *clData) ([]string, error) {
- manifestCommits, err := candidateManifestCommits(client, manifestRepo, targetData)
- if err != nil {
- return nil, err
+func candidateBuildNums(client gitilesProto.GitilesClient, manifestRepo string, targetData *clData) ([]string, utils.ChangelogError) {
+ manifestCommits, utilErr := candidateManifestCommits(client, manifestRepo, targetData)
+ if utilErr != nil {
+ return nil, utilErr
}
tags, err := repoTags(client, manifestRepo)
if err != nil {
- return nil, fmt.Errorf("tagMapping: Failed to retrieve tags for project %s:\n%w", manifestRepo, err)
+ log.Errorf("tagMapping: Failed to retrieve tags for project %s:\n%v", manifestRepo, err)
+ httpCode := utils.GerritErrCode(err)
+ return nil, utils.FromFindBuildError(httpCode, targetData.CLNum)
}
log.Debug("Retrieving associated build number for each manifest commit")
gitTagsMap := map[string]string{}
@@ -285,10 +291,11 @@
for i, commit := range manifestCommits {
tag, ok := gitTagsMap[commit.Id]
if !ok {
- return nil, fmt.Errorf("candidateBuildNums: No ref tag found for commit sha %s in repository %s", commit.Id, manifestRepo)
+ log.Errorf("candidateBuildNums: No ref tag found for commit sha %s in repository %s", commit.Id, manifestRepo)
+ return nil, utils.InternalError
} else if len(tag) <= 10 {
- // Git tags should be of the form refs/tags/<buildNum>
- return nil, fmt.Errorf("candidateBuildNums: Ref tag: %s for commit sha %s is malformed", tag, commit.Id)
+ log.Errorf("candidateBuildNums: Ref tag: %s for commit sha %s is malformed", tag, commit.Id)
+ return nil, utils.InternalError
}
// Remove refs/tags/ prefix for each git tag
output[i] = gitTagsMap[commit.Id][10:]
@@ -349,7 +356,7 @@
// getRepoData retrieves information about the repository being modified by the
// CL. It retrieves candidate build numbers and their associated SHA, the
// the first and last SHA in the repository changelog, and the remote URL.
-func getRepoData(client gitilesProto.GitilesClient, manifestRepo string, targetData *clData) (*repoData, error) {
+func getRepoData(client gitilesProto.GitilesClient, manifestRepo string, targetData *clData) (*repoData, utils.ChangelogError) {
buildNums, err := candidateBuildNums(client, manifestRepo, targetData)
if err != nil {
return nil, err
@@ -377,7 +384,8 @@
continue
}
if output.RemoteURL != "" && output.RemoteURL != curr.RemoteURL {
- return nil, fmt.Errorf("getRepoData: Remote URL for repository %s changed in build %s", targetData.Project, curr.BuildNum)
+ log.Errorf("getRepoData: Remote URL for repository %s changed in build %s", targetData.Project, curr.BuildNum)
+ return nil, utils.InternalError
}
output.RemoteURL = curr.RemoteURL
// Since a manifest file may not use the repository/branch used by a
@@ -395,14 +403,15 @@
}
}
if len(output.Candidates) == 0 {
- return nil, ErrorNoManifestBranchMatch
+ log.Debugf("getRepoData: No builds found for CL %s", targetData.CLNum)
+ return nil, utils.FromFindBuildError("404", targetData.CLNum)
}
return &output, nil
}
// firstBuild retrieves the earliest build containing the target CL from a map
// of candidate builds.
-func firstBuild(changelog []*git.Commit, targetData *clData, candidates map[string]string) (string, error) {
+func firstBuild(changelog []*git.Commit, targetData *clData, candidates map[string]string) (string, utils.ChangelogError) {
log.Debug("Scanning changelog for first build")
targetIdx := -1
for i, commit := range changelog {
@@ -411,7 +420,7 @@
}
}
if targetIdx == -1 {
- return "", ErrorCLNotLanded
+ return "", utils.FromFindBuildError("404", targetData.CLNum)
}
for i := targetIdx; i >= 0; i-- {
currSHA := changelog[i].Id
@@ -419,51 +428,56 @@
return buildNum, nil
}
}
- return "", ErrorCLNotLanded
+ return "", utils.FromFindBuildError("404", targetData.CLNum)
}
// FindBuild locates the first build that a CL was introduced to.
-func FindBuild(request *BuildRequest) (*BuildResponse, error) {
+func FindBuild(request *BuildRequest) (*BuildResponse, utils.ChangelogError) {
log.Debugf("Fetching first build for CL: %s", request.CL)
- if request == nil {
- return nil, fmt.Errorf("FindCL: request is nil")
- }
start := time.Now()
+ if request == nil {
+ log.Error("expected non-nil request")
+ return nil, utils.InternalError
+ }
gerritClient, err := gerrit.NewClient(request.HTTPClient, request.GerritHost)
if err != nil {
- return nil, fmt.Errorf("FindCL: Error creating Gerrit Client:\n%w", err)
+ log.Errorf("failed to establish Gerrit client for host %s:\n%v", request.GerritHost, err)
+ return nil, utils.InternalError
}
gitilesClient, err := gitilesApi.NewRESTClient(request.HTTPClient, request.GitilesHost, true)
if err != nil {
- return nil, fmt.Errorf("FindCL: Error creating Gitiles Client:\n%w", err)
+ log.Errorf("failed to establish Gitiles client for host %s:\n%v", request.GitilesHost, err)
+ return nil, utils.InternalError
}
- clData, err := getCLData(gerritClient, request.CL, request.RepoPrefix)
- if err != nil {
- return nil, fmt.Errorf("FindCL: Error retrieving CL for changeID: %s:\n%w", request.CL, err)
+ clData, clErr := getCLData(gerritClient, request.CL, request.RepoPrefix)
+ if clErr != nil {
+ return nil, clErr
}
- repoData, err := getRepoData(gitilesClient, request.ManifestRepo, clData)
- if err != nil {
- return nil, err
+ repoData, clErr := getRepoData(gitilesClient, request.ManifestRepo, clData)
+ if clErr != nil {
+ return nil, clErr
}
// The remote URL for a repo may not be the same as the manifest remote URL
if request.GitilesHost != repoData.RemoteURL {
log.Debugf("Repository is located in different GoB host, setting gitiles client to URL: %s", repoData.RemoteURL)
gitilesClient, err = gitilesApi.NewRESTClient(request.HTTPClient, repoData.RemoteURL, true)
if err != nil {
- return nil, fmt.Errorf("FindCL: Failed to create gitiles client for repo remote URL: %s\n%w", repoData.RemoteURL, err)
+ log.Errorf("failed to establish Gitiles client for host %s:\n%v", repoData.RemoteURL, err)
+ return nil, utils.InternalError
}
}
changelog, _, err := utils.Commits(gitilesClient, clData.Project, repoData.TargetSHA, repoData.SourceSHA, -1)
if err != nil {
- return nil, err
+ httpCode := utils.GerritErrCode(err)
+ return nil, utils.FromFindBuildError(httpCode, clData.CLNum)
}
- buildNum, err := firstBuild(changelog, clData, repoData.Candidates)
- if err != nil {
- return nil, err
+ buildNum, clErr := firstBuild(changelog, clData, repoData.Candidates)
+ if clErr != nil {
+ return nil, clErr
}
log.Debugf("Retrieved first build for CL: %s in %s\n", request.CL, time.Since(start))
return &BuildResponse{
BuildNum: buildNum,
- CLNum: strconv.Itoa(clData.CLNum),
+ CLNum: clData.CLNum,
}, nil
}
diff --git a/src/pkg/findbuild/findbuild_test.go b/src/pkg/findbuild/findbuild_test.go
index 31b6b8e..bf8b13c 100644
--- a/src/pkg/findbuild/findbuild_test.go
+++ b/src/pkg/findbuild/findbuild_test.go
@@ -16,7 +16,6 @@
import (
"context"
- "errors"
"fmt"
"net/http"
"testing"
@@ -34,14 +33,6 @@
fallbackRepoPrefix string = "mirrors/cros/"
)
-func unwrappedError(err error) error {
- innerErr := err
- for errors.Unwrap(innerErr) != nil {
- innerErr = errors.Unwrap(innerErr)
- }
- return innerErr
-}
-
func getHTTPClient() (*http.Client, error) {
creds, err := google.FindDefaultCredentials(context.Background(), gerrit.OAuthScope)
if err != nil || len(creds.JSON) == 0 {
@@ -60,7 +51,7 @@
ManifestRepo string
OutputBuildNum string
ShouldFallback bool
- ShouldError bool
+ ExpectedError string
}{
"invalid host": {
Change: "3781",
@@ -68,7 +59,7 @@
GitilesHost: "zop.googlesource.com",
ManifestRepo: externalManifestRepo,
ShouldFallback: false,
- ShouldError: true,
+ ExpectedError: "500",
},
"incorrect manifest repo": {
Change: "3781",
@@ -76,7 +67,7 @@
GitilesHost: externalGitilesURL,
ManifestRepo: "cos/manifest",
ShouldFallback: false,
- ShouldError: true,
+ ExpectedError: "500",
},
"master branch release version": {
Change: "3280",
@@ -85,7 +76,6 @@
ManifestRepo: externalManifestRepo,
OutputBuildNum: "15085.0.0",
ShouldFallback: false,
- ShouldError: false,
},
"R85-13310.B branch release version": {
Change: "3206",
@@ -94,7 +84,6 @@
ManifestRepo: externalManifestRepo,
OutputBuildNum: "13310.1025.0",
ShouldFallback: false,
- ShouldError: false,
},
"only CL in build diff": {
Change: "3781",
@@ -103,7 +92,6 @@
ManifestRepo: externalManifestRepo,
OutputBuildNum: "12371.1072.0",
ShouldFallback: false,
- ShouldError: false,
},
"non-existant CL": {
Change: "9999999999",
@@ -112,7 +100,7 @@
FallbackGerritHost: externalFallbackGerritURL,
ManifestRepo: externalManifestRepo,
ShouldFallback: true,
- ShouldError: true,
+ ExpectedError: "404",
},
"abandoned CL": {
Change: "3743",
@@ -120,7 +108,7 @@
GitilesHost: externalGitilesURL,
ManifestRepo: externalManifestRepo,
ShouldFallback: false,
- ShouldError: true,
+ ExpectedError: "406",
},
"under review CL": {
Change: "1540",
@@ -128,7 +116,7 @@
GitilesHost: externalGitilesURL,
ManifestRepo: externalManifestRepo,
ShouldFallback: false,
- ShouldError: true,
+ ExpectedError: "406",
},
"chromium CL": {
Change: "2288114",
@@ -139,7 +127,6 @@
FallbackPrefix: fallbackRepoPrefix,
OutputBuildNum: "15049.0.0",
ShouldFallback: true,
- ShouldError: false,
},
"use commit SHA": {
Change: "80809c436f1cae4cde117fce34b82f38bdc2fd36",
@@ -149,15 +136,14 @@
ManifestRepo: externalManifestRepo,
OutputBuildNum: "12871.1183.0",
ShouldFallback: false,
- ShouldError: false,
},
- "reject cherry-picked change-id": {
+ "reject change-id": {
Change: "I6cc721e6e61b3863e549045e68c1a2bd363efa0a",
GerritHost: externalGerritURL,
GitilesHost: externalGitilesURL,
ManifestRepo: externalManifestRepo,
ShouldFallback: false,
- ShouldError: true,
+ ExpectedError: "400",
},
"third_party/kernel special branch case": {
Change: "3302",
@@ -166,7 +152,6 @@
ManifestRepo: externalManifestRepo,
OutputBuildNum: "15088.0.0",
ShouldFallback: false,
- ShouldError: false,
},
"branch not in manifest": {
Change: "1592",
@@ -174,7 +159,17 @@
GitilesHost: externalGitilesURL,
ManifestRepo: externalManifestRepo,
ShouldFallback: false,
- ShouldError: true,
+ ExpectedError: "500",
+ },
+ "cl fallback earlier than earliest COS build": {
+ Change: "3740",
+ GerritHost: externalGerritURL,
+ GitilesHost: externalGitilesURL,
+ FallbackGerritHost: externalFallbackGerritURL,
+ ManifestRepo: externalManifestRepo,
+ FallbackPrefix: fallbackRepoPrefix,
+ ShouldFallback: true,
+ ExpectedError: "404",
},
}
@@ -190,11 +185,10 @@
CL: test.Change,
}
res, err := FindBuild(req)
- innerErr := unwrappedError(err)
- if innerErr != ErrorCLNotFound && test.ShouldFallback {
+ if err != nil && err.HTTPCode() != "404" && test.ShouldFallback {
t.Fatalf("expected not found error, got %v", err)
}
- if innerErr == ErrorCLNotFound {
+ if err != nil && err.HTTPCode() == "404" {
fallbackReq := &BuildRequest{
HTTPClient: httpClient,
GerritHost: test.FallbackGerritHost,
@@ -206,13 +200,13 @@
res, err = FindBuild(fallbackReq)
}
switch {
- case (err != nil) != test.ShouldError:
- ShouldError := "no error"
- if test.ShouldError {
- ShouldError = "some error"
- }
- t.Fatalf("expected %s, got: %v", ShouldError, err)
- case !test.ShouldError && res.BuildNum != test.OutputBuildNum:
+ case test.ExpectedError == "" && err != nil:
+ t.Fatalf("expected no error, got %v", err)
+ case test.ExpectedError != "" && err == nil:
+ t.Fatalf("expected error code %s, got nil err", test.ExpectedError)
+ case test.ExpectedError != "" && err != nil && test.ExpectedError != err.HTTPCode():
+ t.Fatalf("expected error code %s, got error code %s", test.ExpectedError, err.HTTPCode())
+ case test.ExpectedError == "" && res.BuildNum != test.OutputBuildNum:
t.Fatalf("expected output %s, got %s", test.OutputBuildNum, res)
}
})
diff --git a/src/pkg/utils/changelogerror.go b/src/pkg/utils/changelogerror.go
new file mode 100644
index 0000000..52cedc5
--- /dev/null
+++ b/src/pkg/utils/changelogerror.go
@@ -0,0 +1,184 @@
+// Copyright 2020 Google Inc. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// This package contains the error interface returned by changelog and
+// findbuild packages. It includes functions to retrieve HTTP status codes
+// from Gerrit and Gitiles errors, and functions to create ChangelogErrors
+// relevant to the changelog and findbuild features.
+
+package utils
+
+import (
+ "errors"
+ "fmt"
+ "regexp"
+
+ log "github.com/sirupsen/logrus"
+ "google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
+)
+
+var (
+ grpcCodeToHTTP = map[string]string{
+ codes.Unknown.String(): "500",
+ codes.InvalidArgument.String(): "400",
+ codes.NotFound.String(): "404",
+ codes.PermissionDenied.String(): "403",
+ codes.Unauthenticated.String(): "401",
+ codes.ResourceExhausted.String(): "429",
+ codes.FailedPrecondition.String(): "400",
+ codes.OutOfRange.String(): "400",
+ codes.Internal.String(): "500",
+ codes.Unavailable.String(): "503",
+ codes.DataLoss.String(): "500",
+ }
+ httpStatus = map[string]string{
+ "400": "400 Bad Request",
+ "401": "401 Unauthorized",
+ "403": "403 Forbidden",
+ "404": "404 Not Found",
+ "406": "406 Not Acceptable",
+ "429": "429 Too Many Requests",
+ "500": "500 Internal Server Error",
+ "503": "503 Service Unavailable",
+ }
+ // Gitiles specific error messages
+ // Specifies a string format to allow the build number to be inserted
+ httpGitilesErrorFmt = map[string]string{
+ "404": "Build number %s not found. Please input a valid build number (example: 13310.1035.0).",
+ }
+ // Gerrit specific error messages
+ // Specifies a string format to allow CL number to be inserted
+ httpGerritErrorFmt = map[string]string{
+ "400": "%s is not a recognized CL identifier. Please enter either a CL-Number (example: 3206) or a Commit-SHA (example: I7e549d7753cc7acec2b44bb5a305347a97719ab9) for a submitted CL.",
+ "404": "No build found for a CL with ID %s. Please enter either the CL-Number (example: 3206) or a Commit-SHA (example: I7e549d7753cc7acec2b44bb5a305347a97719ab9) of a submitted CL.",
+ "406": "CL identifier %s maps to a CL that has not been submitted yet. A CL will not enter any build until it is successfully submitted. Please provide the CL identifier for a submitted CL.",
+ }
+ // Standard error messages
+ httpErrorReplacement = map[string]string{
+ "403": "This account does not have access to internal repositories. Please retry with an authorized account, or select the external button to query from publically accessible builds.",
+ "429": "Our servers are currently experiencing heavy load. Please retry in a couple minutes.",
+ "500": "An unexpected error occurred while retrieving the requested information.",
+ }
+ gitiles403ErrMsg = "unexpected HTTP 403 from Gitiles"
+ gerritErrCodeRe = regexp.MustCompile("status code\\s*(\\d+)")
+
+ // InternalError is a ChangelogError object indicating an internal error
+ InternalError = newError("500", httpErrorReplacement["500"])
+)
+
+// ChangelogError is the error type used by the changelog and findbuild package
+type ChangelogError interface {
+ error
+ HTTPCode() string
+ HTTPStatus() string
+}
+
+// UtilChangelogError implements the ChangelogError interface
+type UtilChangelogError struct {
+ httpCode string
+ httpStatus string
+ err string
+}
+
+// HTTPCode retrieves the HTTP error code associated with the error
+// ex. 400
+func (e *UtilChangelogError) HTTPCode() string {
+ return e.httpCode
+}
+
+// HTTPStatus retrieves the full HTTP status associated with the error
+// ex. 400 Bad Request
+func (e *UtilChangelogError) HTTPStatus() string {
+ return e.httpStatus
+}
+
+func (e *UtilChangelogError) Error() string {
+ return e.err
+}
+
+func unwrapError(err error) error {
+ innerErr := err
+ for errors.Unwrap(innerErr) != nil {
+ innerErr = errors.Unwrap(innerErr)
+ }
+ return innerErr
+}
+
+// newError creates a new UtilChangelogError
+func newError(httpCode, errString string) *UtilChangelogError {
+ output := UtilChangelogError{
+ httpCode: httpCode,
+ err: errString,
+ }
+ if header, ok := httpStatus[httpCode]; ok {
+ output.httpStatus = header
+ } else {
+ log.Errorf("No HTTP status mapping for HTTP code %s", httpCode)
+ output.httpStatus = httpStatus["500"]
+ }
+ return &output
+}
+
+// FromChangelogError creates Changelog errors that are relevant to Changelog
+// functionality.
+func FromChangelogError(httpCode, buildNum string) *UtilChangelogError {
+ if errFmt, ok := httpGitilesErrorFmt[httpCode]; ok {
+ errStr := fmt.Sprintf(errFmt, buildNum)
+ return newError("404", errStr)
+ } else if replacementErr, ok := httpErrorReplacement[httpCode]; ok {
+ return newError(httpCode, replacementErr)
+ }
+ return InternalError
+}
+
+// FromFindBuildError creates Changelog errors that are relevant to FindBuild
+// functionality.
+func FromFindBuildError(httpCode string, clID string) *UtilChangelogError {
+ if errFmt, ok := httpGerritErrorFmt[httpCode]; ok {
+ errStr := fmt.Sprintf(errFmt, clID)
+ return newError(httpCode, errStr)
+ } else if replacementErr, ok := httpErrorReplacement[httpCode]; ok {
+ return newError(httpCode, replacementErr)
+ }
+ return InternalError
+}
+
+// GitilesErrCode parses a Gitiles error message and returns an HTTP error code
+// associated with the error. Returns 500 if no error code is found.
+func GitilesErrCode(err error) string {
+ rpcStatus, ok := status.FromError(err)
+ if !ok {
+ return "500"
+ }
+ code, text := rpcStatus.Code(), rpcStatus.Message()
+ // RPC status code misclassifies 403 error as 500 error for Gitiles requests
+ if code == codes.Internal && text == gitiles403ErrMsg {
+ code = codes.PermissionDenied
+ }
+ if httpCode, ok := grpcCodeToHTTP[code.String()]; ok {
+ return httpCode
+ }
+ return "500"
+}
+
+// GerritErrCode parse a Gerrit error and returns an HTTP error code associated
+// with the error. Returns 500 if no error code is found.
+func GerritErrCode(err error) string {
+ matches := gerritErrCodeRe.FindStringSubmatch(err.Error())
+ if len(matches) != 2 {
+ return "500"
+ }
+ return matches[1]
+}
diff --git a/src/pkg/utils/changelogerror_test.go b/src/pkg/utils/changelogerror_test.go
new file mode 100644
index 0000000..357e9ec
--- /dev/null
+++ b/src/pkg/utils/changelogerror_test.go
@@ -0,0 +1,181 @@
+// Copyright 2020 Google Inc. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package utils
+
+import (
+ "errors"
+ "testing"
+
+ "google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
+)
+
+func TestGerritErrCode(t *testing.T) {
+ tests := map[string]struct {
+ inputErr error
+ expectedCode string
+ }{
+ "Empty Error": {
+ inputErr: errors.New(""),
+ expectedCode: "500",
+ },
+ "Mapped Error Code": {
+ inputErr: errors.New("failed to fetch \"https://cos-internal-review.googlesource.com/a/changes/?n=1&o=CURRENT_REVISION&q=1\", status code 403"),
+ expectedCode: "403",
+ },
+ "Irregular Code": {
+ inputErr: errors.New("failed to fetch \"https://cos-internal-review.googlesource.com/a/changes/?n=1&o=CURRENT_REVISION&q=1\", status code 689"),
+ expectedCode: "689",
+ },
+ }
+ for name, test := range tests {
+ t.Run(name, func(t *testing.T) {
+ code := GerritErrCode(test.inputErr)
+ if code != test.expectedCode {
+ t.Errorf("GerritErrCode failed, expected HTTP code %s, got %s", test.expectedCode, code)
+ }
+ })
+ }
+}
+
+func TestFindBuildError(t *testing.T) {
+ tests := map[string]struct {
+ inputCode string
+ clID string
+ expectedCode string
+ expectedStatus string
+ expectedErrStr string
+ }{
+ "No Code": {
+ inputCode: "500",
+ clID: "1",
+ expectedCode: "500",
+ expectedStatus: "500 Internal Server Error",
+ expectedErrStr: "An unexpected error occurred while retrieving the requested information.",
+ },
+ "Unformatted Error": {
+ inputCode: "403",
+ clID: "1",
+ expectedCode: "403",
+ expectedStatus: "403 Forbidden",
+ expectedErrStr: "This account does not have access to internal repositories. Please retry with an authorized account, or select the external button to query from publically accessible builds.",
+ },
+ "Formatted Error": {
+ inputCode: "404",
+ clID: "1214",
+ expectedCode: "404",
+ expectedStatus: "404 Not Found",
+ expectedErrStr: "No build found for a CL with ID 1214. Please enter either the CL-Number (example: 3206) or a Commit-SHA (example: I7e549d7753cc7acec2b44bb5a305347a97719ab9) of a submitted CL.",
+ },
+ "Unmapped Code": {
+ inputCode: "500",
+ clID: "1",
+ expectedCode: "500",
+ expectedStatus: "500 Internal Server Error",
+ expectedErrStr: "An unexpected error occurred while retrieving the requested information.",
+ },
+ }
+ for name, test := range tests {
+ t.Run(name, func(t *testing.T) {
+ err := FromFindBuildError(test.inputCode, test.clID)
+ if err.HTTPCode() != test.expectedCode {
+ t.Errorf("FromFindBuildError failed, expected HTTP code %s, got %s", test.expectedCode, err.HTTPCode())
+ } else if err.HTTPStatus() != test.expectedStatus {
+ t.Errorf("FromFindBuildError failed, expected HTTP status %s, got %s", test.expectedStatus, err.HTTPStatus())
+ } else if err.Error() != test.expectedErrStr {
+ t.Errorf("FromFindBuildError failed, expected error string %s, got %s", test.expectedErrStr, err.Error())
+ }
+ })
+ }
+}
+
+func TestGitilesErrCode(t *testing.T) {
+ tests := map[string]struct {
+ inputErr error
+ expectedCode string
+ }{
+ "Default Error Code": {
+ inputErr: errors.New("code = e desc = not a desc"),
+ expectedCode: "500",
+ },
+ "Mapped Error Code": {
+ inputErr: status.New(codes.NotFound, "not found").Err(),
+ expectedCode: "404",
+ },
+ "403 Code Edge Case": {
+ inputErr: status.New(codes.Internal, "unexpected HTTP 403 from Gitiles").Err(),
+ expectedCode: "403",
+ },
+ }
+ for name, test := range tests {
+ t.Run(name, func(t *testing.T) {
+ code := GitilesErrCode(test.inputErr)
+ if code != test.expectedCode {
+ t.Errorf("GitilesErrCode failed, expected HTTP code %s, got %s", test.expectedCode, code)
+ }
+ })
+ }
+}
+
+func TestFromChangelogError(t *testing.T) {
+ tests := map[string]struct {
+ inputCode string
+ clID string
+ expectedCode string
+ expectedStatus string
+ expectedErrStr string
+ }{
+ "No Code": {
+ inputCode: "500",
+ clID: "1",
+ expectedCode: "500",
+ expectedStatus: "500 Internal Server Error",
+ expectedErrStr: "An unexpected error occurred while retrieving the requested information.",
+ },
+ "Unformatted Error": {
+ inputCode: "403",
+ clID: "1",
+ expectedCode: "403",
+ expectedStatus: "403 Forbidden",
+ expectedErrStr: "This account does not have access to internal repositories. Please retry with an authorized account, or select the external button to query from publically accessible builds.",
+ },
+ "Formatted Error": {
+ inputCode: "404",
+ clID: "15000.1.0",
+ expectedCode: "404",
+ expectedStatus: "404 Not Found",
+ expectedErrStr: "Build number 15000.1.0 not found. Please input a valid build number (example: 13310.1035.0).",
+ },
+ "Unmapped Code": {
+ inputCode: "500",
+ clID: "1",
+ expectedCode: "500",
+ expectedStatus: "500 Internal Server Error",
+ expectedErrStr: "An unexpected error occurred while retrieving the requested information.",
+ },
+ }
+ for name, test := range tests {
+ t.Run(name, func(t *testing.T) {
+ err := FromChangelogError(test.inputCode, test.clID)
+ if err.HTTPCode() != test.expectedCode {
+ t.Errorf("FromChangelogError failed, expected HTTP code %s, got %s", test.expectedCode, err.HTTPCode())
+ } else if err.HTTPStatus() != test.expectedStatus {
+ t.Errorf("FromChangelogError failed, expected HTTP status %s, got %s", test.expectedStatus, err.HTTPStatus())
+ } else if err.Error() != test.expectedErrStr {
+ t.Errorf("FromChangelogError failed, expected error string %s, got %s", test.expectedErrStr, err.Error())
+ }
+ })
+ }
+}
