Merge pull request #27 from itsdalmo/human_readable_version

Human readable versions

README.md

@@ -29,12 +29,12 @@ Note: If `v3_endpoint` is set, `v4_endpoint` must also be set (and the other way
 
 #### `check`
 
-Produces new versions for all commits (after the last version) ordered by the push date.
+Produces new versions for all commits (after the last version) ordered by the committed date.
 A version is represented as follows:
 
-- `pr`: The subject ID of the pull request.
-- `commit`: The subject ID of the last commit on the Pullrequest.
-- `pushed`: Timestamp of when the commit was pushed (and webhook triggered). Used to filter subsequent checks.
+- `pr`: The pull request number.
+- `commit`: The commit SHA.
+- `committed`: Timestamp of when the commit was committed. Used to filter subsequent checks.
 
 If several commits are pushed to a given PR at the same time, the last commit will be the new version.
 
@@ -127,13 +127,13 @@ jobs:
 The Github API(s) have a rate limit of 5000 requests per hour (per user). This resource will incur the following costs:
 
 - `check`: Minimum 1, max 1 per 100th *open* pull request.
-- `in`: Fixed cost of 2. Fetches PR and Commit from global ID (passed in via version).
+- `in`: Fixed cost of 1. Fetches the pull request at the given commit.
 - `out`: Minimum 1, max 3 (1 for each of `status`, `comment` and `comment_file`).
 
 E.g., typical use for a repository with 125 open pull requests will incur the following costs for every commit:
 
 - `check`: 2 (paginate 125 PR's with 100 per page)
-- `in`: 2 (fetch commit and PR from global ID's)
+- `in`: 1 (fetch the pull request at the given commit ref)
 - `out`: 1 (set status on the commit)
 
-With a rate limit of 5000 per hour, it could handle 1000 commits between all of the 125 open pull requests in the span of that hour.
+With a rate limit of 5000 per hour, it could handle 1250 commits between all of the 125 open pull requests in the span of that hour.

check_test.go

@@ -11,22 +11,10 @@ import (
 
 var (
 	testPullRequests = []*resource.PullRequest{
-		{
-			PullRequestObject: createTestPR(1),
-			Tip:               createTestCommit(1, true),
-		},
-		{
-			PullRequestObject: createTestPR(2),
-			Tip:               createTestCommit(2, false),
-		},
-		{
-			PullRequestObject: createTestPR(3),
-			Tip:               createTestCommit(3, false),
-		},
-		{
-			PullRequestObject: createTestPR(4),
-			Tip:               createTestCommit(4, false),
-		},
+		createTestPR(1, true),
+		createTestPR(2, false),
+		createTestPR(3, false),
+		createTestPR(4, false),
 	}
 )
 

e2e/e2e_test.go

@@ -14,11 +14,11 @@ import (
 )
 
 var (
-	targetCommitID      = "MDY6Q29tbWl0MTMyNTc2MjQ1OmE1MTE0ZjZhYjg5ZjRiNzM2NjU1NjQyYTExZThkMTVjZTM2M2Q4ODI="
-	targetPullRequestID = "MDExOlB1bGxSZXF1ZXN0MTg3Mzg4MDE0"
+	targetCommitID      = "a5114f6ab89f4b736655642a11e8d15ce363d882"
+	targetPullRequestID = "4"
 	targetDateTime      = time.Date(2018, time.May, 11, 8, 43, 48, 0, time.UTC)
-	latestCommitID      = "MDY6Q29tbWl0MTMyNTc2MjQ1Ojg5MGE3ZTRmMGQ1YjA1YmRhOGVhMjFiOTFmNDYwNGUzZTAzMTM1ODE="
-	latestPullRequestID = "MDExOlB1bGxSZXF1ZXN0MTg3Nzg4NjAy"
+	latestCommitID      = "890a7e4f0d5b05bda8ea21b91f4604e3e0313581"
+	latestPullRequestID = "5"
 	latestDateTime      = time.Date(2018, time.May, 14, 10, 51, 58, 0, time.UTC)
 )
 
@@ -160,7 +160,7 @@ func TestGetAndPutE2E(t *testing.T) {
 			directory:      dir,
 			getParameters:  resource.GetParameters{},
 			putParameters:  resource.PutParameters{},
-			versionString:  `{"pr":"MDExOlB1bGxSZXF1ZXN0MTg3Mzg4MDE0","commit":"MDY6Q29tbWl0MTMyNTc2MjQ1OmE1MTE0ZjZhYjg5ZjRiNzM2NjU1NjQyYTExZThkMTVjZTM2M2Q4ODI=","committed":"0001-01-01T00:00:00Z"}`,
+			versionString:  `{"pr":"4","commit":"a5114f6ab89f4b736655642a11e8d15ce363d882","committed":"0001-01-01T00:00:00Z"}`,
 			metadataString: `[{"name":"pr","value":"4"},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/4"},{"name":"head_sha","value":"a5114f6ab89f4b736655642a11e8d15ce363d882"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"Push 2."},{"name":"author","value":"itsdalmo"}]`,
 		},
 	}

github.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"net/url"
 	"os"
+	"strconv"
 	"strings"
 
 	"github.com/google/go-github/github"
@@ -19,8 +20,7 @@ type Github interface {
 	ListOpenPullRequests() ([]*PullRequest, error)
 	ListModifiedFiles(int) ([]string, error)
 	PostComment(string, string) error
-	GetPullRequestByID(string) (*PullRequestObject, error)
-	GetCommitByID(string) (*CommitObject, error)
+	GetPullRequest(string, string) (*PullRequest, error)
 	UpdateCommitStatus(string, string, string) error
 }
 
@@ -180,48 +180,55 @@ func (m *GithubClient) PostComment(objectID, comment string) error {
 	return err
 }
 
-// GetPullRequestByID ...
-func (m *GithubClient) GetPullRequestByID(objectID string) (*PullRequestObject, error) {
-	var query struct {
-		Node struct {
-			PullRequest PullRequestObject `graphql:"... on PullRequest"`
-		} `graphql:"node(id:$nodeId)"`
-	}
-
-	vars := map[string]interface{}{
-		"nodeId": githubv4.ID(objectID),
-	}
-	if err := m.V4.Query(context.TODO(), &query, vars); err != nil {
-		return nil, err
+// GetPullRequest ...
+func (m *GithubClient) GetPullRequest(prNumber, commitRef string) (*PullRequest, error) {
+	pr, err := strconv.Atoi(prNumber)
+	if err != nil {
+		return nil, fmt.Errorf("failed to convert pull request number to int: %s", err)
 	}
-	return &query.Node.PullRequest, nil
-}
 
-// GetCommitByID ...
-func (m *GithubClient) GetCommitByID(objectID string) (*CommitObject, error) {
 	var query struct {
-		Node struct {
-			Commit CommitObject `graphql:"... on Commit"`
-		} `graphql:"node(id:$nodeId)"`
+		Repository struct {
+			PullRequest struct {
+				PullRequestObject
+				Commits struct {
+					Edges []struct {
+						Node struct {
+							Commit CommitObject
+						}
+					}
+				} `graphql:"commits(last:$commitsLast)"`
+			} `graphql:"pullRequest(number:$prNumber)"`
+		} `graphql:"repository(owner:$repositoryOwner,name:$repositoryName)"`
 	}
 
 	vars := map[string]interface{}{
-		"nodeId": githubv4.ID(objectID),
+		"repositoryOwner": githubv4.String(m.Owner),
+		"repositoryName":  githubv4.String(m.Repository),
+		"prNumber":        githubv4.Int(pr),
+		"commitsLast":     githubv4.Int(100),
 	}
+
+	// TODO: Pagination - in case someone pushes > 100 commits before the build has time to start :p
 	if err := m.V4.Query(context.TODO(), &query, vars); err != nil {
 		return nil, err
 	}
-	return &query.Node.Commit, nil
+	for _, c := range query.Repository.PullRequest.Commits.Edges {
+		if c.Node.Commit.OID == commitRef {
+			// Return as soon as we find the correct ref.
+			return &PullRequest{
+				PullRequestObject: query.Repository.PullRequest.PullRequestObject,
+				Tip:               c.Node.Commit,
+			}, nil
+		}
+	}
+
+	// Return an error if the commit was not found
+	return nil, fmt.Errorf("commit with ref '%s' does not exist", commitRef)
 }
 
 // UpdateCommitStatus for a given commit (not supported by V4 API).
-func (m *GithubClient) UpdateCommitStatus(objectID, statusContext, status string) error {
-	commit, err := m.GetCommitByID(objectID)
-	if err != nil {
-		return err
-	}
-
-	// Format context
+func (m *GithubClient) UpdateCommitStatus(commitRef, statusContext, status string) error {
 	c := []string{"concourse-ci"}
 	if statusContext == "" {
 		c = append(c, "status")
@@ -236,11 +243,11 @@ func (m *GithubClient) UpdateCommitStatus(objectID, statusContext, status string
 		build = strings.Join([]string{build, "builds", os.Getenv("BUILD_ID")}, "/")
 	}
 
-	_, _, err = m.V3.Repositories.CreateStatus(
+	_, _, err := m.V3.Repositories.CreateStatus(
 		context.TODO(),
 		m.Owner,
 		m.Repository,
-		commit.OID,
+		commitRef,
 		&github.RepoStatus{
 			State:       github.String(strings.ToLower(status)),
 			TargetURL:   github.String(build),

in.go

@@ -11,15 +11,10 @@ import (
 
 // Get (business logic)
 func Get(request GetRequest, github Github, git Git, outputDir string) (*GetResponse, error) {
-	commit, err := github.GetCommitByID(request.Version.Commit)
+	pull, err := github.GetPullRequest(request.Version.PR, request.Version.Commit)
 	if err != nil {
 		return nil, fmt.Errorf("failed to retrieve pull request: %s", err)
 	}
-	pr, err := github.GetPullRequestByID(request.Version.PR)
-	if err != nil {
-		return nil, fmt.Errorf("failed to retrieve pull request: %s", err)
-	}
-	pull := &PullRequest{PullRequestObject: *pr, Tip: *commit}
 
 	// Clone the repository and fetch the PR
 	if err := git.Init(); err != nil {
@@ -48,10 +43,10 @@ func Get(request GetRequest, github Github, git Git, outputDir string) (*GetResp
 	var metadata Metadata
 	metadata.Add("pr", strconv.Itoa(pull.Number))
 	metadata.Add("url", pull.URL)
-	metadata.Add("head_sha", commit.OID)
+	metadata.Add("head_sha", pull.Tip.OID)
 	metadata.Add("base_sha", baseSHA)
-	metadata.Add("message", commit.Message)
-	metadata.Add("author", commit.Author.User.Login)
+	metadata.Add("message", pull.Tip.Message)
+	metadata.Add("author", pull.Tip.Author.User.Login)
 
 	// Write version and metadata for reuse in PUT
 	path := filepath.Join(outputDir, ".git", "resource")

in_test.go

@@ -23,8 +23,7 @@ func TestGet(t *testing.T) {
 		source         resource.Source
 		version        resource.Version
 		parameters     resource.GetParameters
-		pullRequest    resource.PullRequestObject
-		commit         resource.CommitObject
+		pullRequest    *resource.PullRequest
 		versionString  string
 		metadataString string
 	}{
@@ -40,8 +39,7 @@ func TestGet(t *testing.T) {
 				CommittedDate: time.Time{},
 			},
 			parameters:     resource.GetParameters{},
-			pullRequest:    createTestPR(1),
-			commit:         createTestCommit(1, false),
+			pullRequest:    createTestPR(1, false),
 			versionString:  `{"pr":"pr1","commit":"commit1","committed":"0001-01-01T00:00:00Z"}`,
 			metadataString: `[{"name":"pr","value":"1"},{"name":"url","value":"pr1 url"},{"name":"head_sha","value":"oid1"},{"name":"base_sha","value":"sha"},{"name":"message","value":"commit message1"},{"name":"author","value":"login1"}]`,
 		},
@@ -53,8 +51,7 @@ func TestGet(t *testing.T) {
 			defer ctrl.Finish()
 
 			github := mocks.NewMockGithub(ctrl)
-			github.EXPECT().GetPullRequestByID(tc.version.PR).Times(1).Return(&tc.pullRequest, nil)
-			github.EXPECT().GetCommitByID(tc.version.Commit).Times(1).Return(&tc.commit, nil)
+			github.EXPECT().GetPullRequest(tc.version.PR, tc.version.Commit).Times(1).Return(tc.pullRequest, nil)
 
 			git := mocks.NewMockGit(ctrl)
 			gomock.InOrder(
@@ -63,7 +60,7 @@ func TestGet(t *testing.T) {
 				git.EXPECT().Fetch(tc.pullRequest.Repository.URL, tc.pullRequest.Number).Times(1).Return(nil),
 				git.EXPECT().RevParse(tc.pullRequest.BaseRefName).Times(1).Return("sha", nil),
 				git.EXPECT().Checkout("sha").Times(1).Return(nil),
-				git.EXPECT().Merge(tc.commit.OID).Times(1).Return(nil),
+				git.EXPECT().Merge(tc.pullRequest.Tip.OID).Times(1).Return(nil),
 			)
 
 			dir := createTestDirectory(t)
@@ -93,38 +90,35 @@ func TestGet(t *testing.T) {
 	}
 }
 
-func createTestPR(count int) resource.PullRequestObject {
-	n := strconv.Itoa(count)
-
-	return resource.PullRequestObject{
-		ID:          fmt.Sprintf("pr%s", n),
-		Number:      count,
-		Title:       fmt.Sprintf("pr%s title", n),
-		URL:         fmt.Sprintf("pr%s url", n),
-		BaseRefName: "master",
-		HeadRefName: fmt.Sprintf("pr%s", n),
-		Repository: struct{ URL string }{
-			URL: fmt.Sprintf("repo%s url", n),
-		},
-	}
-}
-
-func createTestCommit(count int, skipCI bool) resource.CommitObject {
+func createTestPR(count int, skipCI bool) *resource.PullRequest {
 	n := strconv.Itoa(count)
 	d := time.Now().AddDate(0, 0, -count)
 	m := fmt.Sprintf("commit message%s", n)
 	if skipCI {
 		m = "[skip ci]" + m
 	}
 
-	return resource.CommitObject{
-		ID:            fmt.Sprintf("commit%s", n),
-		OID:           fmt.Sprintf("oid%s", n),
-		CommittedDate: githubv4.DateTime{Time: d},
-		Message:       m,
-		Author: struct{ User struct{ Login string } }{
-			User: struct{ Login string }{
-				Login: fmt.Sprintf("login%s", n),
+	return &resource.PullRequest{
+		PullRequestObject: resource.PullRequestObject{
+			ID:          fmt.Sprintf("pr%s", n),
+			Number:      count,
+			Title:       fmt.Sprintf("pr%s title", n),
+			URL:         fmt.Sprintf("pr%s url", n),
+			BaseRefName: "master",
+			HeadRefName: fmt.Sprintf("pr%s", n),
+			Repository: struct{ URL string }{
+				URL: fmt.Sprintf("repo%s url", n),
+			},
+		},
+		Tip: resource.CommitObject{
+			ID:            fmt.Sprintf("commit%s", n),
+			OID:           fmt.Sprintf("oid%s", n),
+			CommittedDate: githubv4.DateTime{Time: d},
+			Message:       m,
+			Author: struct{ User struct{ Login string } }{
+				User: struct{ Login string }{
+					Login: fmt.Sprintf("login%s", n),
+				},
 			},
 		},
 	}

models.go

@@ -2,6 +2,7 @@ package resource
 
 import (
 	"errors"
+	"strconv"
 	"time"
 
 	"github.com/shurcooL/githubv4"
@@ -59,8 +60,8 @@ type Version struct {
 // NewVersion constructs a new Version.
 func NewVersion(p *PullRequest) Version {
 	return Version{
-		PR:            p.ID,
-		Commit:        p.Tip.ID,
+		PR:            strconv.Itoa(p.Number),
+		Commit:        p.Tip.OID,
 		CommittedDate: p.Tip.CommittedDate.Time,
 	}
 }