[bridge/github] improve comments and documentation

[bridge/github] improve error handling and tests

Co-Authored-By: Michael Muré <[email protected]>

Author: a-hilaly

bridge/core/bridge.go

@@ -313,5 +313,5 @@ func (b *Bridge) ExportAll(since time.Time) (<-chan ExportResult, error) {
 		return nil, err
 	}
 
-	return exporter.ExportAll(b.repo, since), nil
+	return exporter.ExportAll(b.repo, since)
 }

bridge/core/interfaces.go

@@ -34,5 +34,5 @@ type Importer interface {
 
 type Exporter interface {
 	Init(conf Configuration) error
-	ExportAll(repo *cache.RepoCache, since time.Time) <-chan ExportResult
+	ExportAll(repo *cache.RepoCache, since time.Time) (<-chan ExportResult, error)
 }

bridge/github/export.go

@@ -40,7 +40,7 @@ type githubExporter struct {
 
 	// cache identifiers used to speed up exporting operations
 	// cleared for each bug
-	cachedIDs map[string]string
+	cachedOperationIDs map[string]string
 
 	// cache labels used to speed up exporting labels events
 	cachedLabels map[string]string
@@ -52,7 +52,7 @@ func (ge *githubExporter) Init(conf core.Configuration) error {
 	//TODO: initialize with multiple tokens
 	ge.identityToken = make(map[string]string)
 	ge.identityClient = make(map[string]*githubv4.Client)
-	ge.cachedIDs = make(map[string]string)
+	ge.cachedOperationIDs = make(map[string]string)
 	ge.cachedLabels = make(map[string]string)
 	return nil
 }
@@ -74,7 +74,7 @@ func (ge *githubExporter) allowOrigin(origin string) bool {
 	return false
 }
 
-// getIdentityClient return an identity github api v4 client
+// getIdentityClient return a githubv4 API client configured with the access token of the given identity.
 // if no client were found it will initialize it from the known tokens map and cache it for next use
 func (ge *githubExporter) getIdentityClient(id string) (*githubv4.Client, error) {
 	client, ok := ge.identityClient[id]
@@ -97,31 +97,29 @@ func (ge *githubExporter) getIdentityClient(id string) (*githubv4.Client, error)
 }
 
 // ExportAll export all event made by the current user to Github
-func (ge *githubExporter) ExportAll(repo *cache.RepoCache, since time.Time) <-chan core.ExportResult {
+func (ge *githubExporter) ExportAll(repo *cache.RepoCache, since time.Time) (<-chan core.ExportResult, error) {
 	out := make(chan core.ExportResult)
 
-	go func(out chan<- core.ExportResult) {
-		defer close(out)
+	user, err := repo.GetUserIdentity()
+	if err != nil {
+		return nil, err
+	}
 
-		user, err := repo.GetUserIdentity()
-		if err != nil {
-			out <- core.NewExportError(err, "")
-			return
-		}
+	ge.identityToken[user.Id()] = ge.conf[keyToken]
 
-		ge.identityToken[user.Id()] = ge.conf[keyToken]
+	// get repository node id
+	ge.repositoryID, err = getRepositoryNodeID(
+		ge.conf[keyOwner],
+		ge.conf[keyProject],
+		ge.conf[keyToken],
+	)
 
-		// get repository node id
-		ge.repositoryID, err = getRepositoryNodeID(
-			ge.conf[keyOwner],
-			ge.conf[keyProject],
-			ge.conf[keyToken],
-		)
+	if err != nil {
+		return nil, err
+	}
 
-		if err != nil {
-			out <- core.NewExportError(err, ge.repositoryID)
-			return
-		}
+	go func() {
+		defer close(out)
 
 		var allIdentitiesIds []string
 		for id := range ge.identityToken {
@@ -140,6 +138,7 @@ func (ge *githubExporter) ExportAll(repo *cache.RepoCache, since time.Time) <-ch
 			snapshot := b.Snapshot()
 
 			// ignore issues created before since date
+			// TODO: compare the Lamport time instead of using the unix time
 			if snapshot.CreatedAt.Before(since) {
 				out <- core.NewExportNothing(b.Id(), "bug created before the since date")
 				continue
@@ -152,9 +151,9 @@ func (ge *githubExporter) ExportAll(repo *cache.RepoCache, since time.Time) <-ch
 				out <- core.NewExportNothing(id, "not an actor")
 			}
 		}
-	}(out)
+	}()
 
-	return out
+	return out, nil
 }
 
 // exportBug publish bugs and related events
@@ -176,7 +175,7 @@ func (ge *githubExporter) exportBug(b *cache.BugCache, since time.Time, out chan
 	// skip bug if origin is not allowed
 	origin, ok := createOp.GetMetadata(keyOrigin)
 	if ok && !ge.allowOrigin(origin) {
-		out <- core.NewExportNothing(b.Id(), fmt.Sprintf("issue taged with origin: %s", origin))
+		out <- core.NewExportNothing(b.Id(), fmt.Sprintf("issue tagged with origin: %s", origin))
 		return
 	}
 
@@ -186,7 +185,8 @@ func (ge *githubExporter) exportBug(b *cache.BugCache, since time.Time, out chan
 		githubURL, ok := createOp.GetMetadata(keyGithubUrl)
 		if !ok {
 			// if we find github ID, github URL must be found too
-			panic("expected to find github issue URL")
+			err := fmt.Errorf("expected to find github issue URL")
+			out <- core.NewExportError(err, b.Id())
 		}
 
 		out <- core.NewExportNothing(b.Id(), "bug already exported")
@@ -199,9 +199,6 @@ func (ge *githubExporter) exportBug(b *cache.BugCache, since time.Time, out chan
 		client, err := ge.getIdentityClient(author.Id())
 		if err != nil {
 			// if bug is still not exported and we do not have the author stop the execution
-
-			// fmt.Println("warning: skipping issue due to missing token for bug creator")
-			// this is not an error, don't export bug
 			out <- core.NewExportNothing(b.Id(), fmt.Sprintf("missing author token"))
 			return
 		}
@@ -252,7 +249,7 @@ func (ge *githubExporter) exportBug(b *cache.BugCache, since time.Time, out chan
 	bugCreationHash = hash.String()
 
 	// cache operation github id
-	ge.cachedIDs[bugCreationHash] = bugGithubID
+	ge.cachedOperationIDs[bugCreationHash] = bugGithubID
 
 	for _, op := range snapshot.Operations[1:] {
 		// ignore SetMetadata operations
@@ -268,10 +265,10 @@ func (ge *githubExporter) exportBug(b *cache.BugCache, since time.Time, out chan
 			return
 		}
 
-		// ignore imported (or exported) operations from github
+		// ignore operations already existing in github (due to import or export)
 		// cache the ID of already exported or imported issues and events from Github
 		if id, ok := op.GetMetadata(keyGithubId); ok {
-			ge.cachedIDs[hash.String()] = id
+			ge.cachedOperationIDs[hash.String()] = id
 			out <- core.NewExportNothing(hash.String(), "already exported operation")
 			continue
 		}
@@ -299,7 +296,7 @@ func (ge *githubExporter) exportBug(b *cache.BugCache, since time.Time, out chan
 			out <- core.NewExportComment(hash.String())
 
 			// cache comment id
-			ge.cachedIDs[hash.String()] = id
+			ge.cachedOperationIDs[hash.String()] = id
 
 		case *bug.EditCommentOperation:
 
@@ -324,7 +321,7 @@ func (ge *githubExporter) exportBug(b *cache.BugCache, since time.Time, out chan
 			} else {
 
 				// case comment edition operation: we need to edit the Github comment
-				commentID, ok := ge.cachedIDs[targetHash]
+				commentID, ok := ge.cachedOperationIDs[targetHash]
 				if !ok {
 					panic("unexpected error: comment id not found")
 				}
@@ -424,7 +421,7 @@ func getRepositoryNodeID(owner, project, token string) (string, error) {
 	}
 
 	if resp.StatusCode != http.StatusOK {
-		return "", fmt.Errorf("error retrieving repository node id %v", resp.StatusCode)
+		return "", fmt.Errorf("HTTP error %v retrieving repository node id", resp.StatusCode)
 	}
 
 	aux := struct {
@@ -668,9 +665,15 @@ func updateGithubIssueStatus(gc *githubv4.Client, id string, status bug.Status)
 	m := &updateIssueMutation{}
 
 	// set state
-	state := githubv4.IssueStateClosed
-	if status == bug.OpenStatus {
+	var state githubv4.IssueState
+
+	switch status {
+	case bug.OpenStatus:
+		state = githubv4.IssueStateOpen
+	case bug.ClosedStatus:
 		state = githubv4.IssueStateOpen
+	default:
+		panic("unknown bug state")
 	}
 
 	input := githubv4.UpdateIssueInput{

bridge/github/export_test.go

@@ -29,102 +29,66 @@ type testCase struct {
 	numOrOp int // number of original operations
 }
 
-func testCases(repo *cache.RepoCache, identity *cache.IdentityCache) ([]*testCase, error) {
+func testCases(t *testing.T, repo *cache.RepoCache, identity *cache.IdentityCache) []*testCase {
 	// simple bug
 	simpleBug, _, err := repo.NewBug("simple bug", "new bug")
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	// bug with comments
 	bugWithComments, _, err := repo.NewBug("bug with comments", "new bug")
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	_, err = bugWithComments.AddComment("new comment")
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	// bug with label changes
 	bugLabelChange, _, err := repo.NewBug("bug label change", "new bug")
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	_, _, err = bugLabelChange.ChangeLabels([]string{"bug"}, nil)
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	_, _, err = bugLabelChange.ChangeLabels([]string{"core"}, nil)
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	_, _, err = bugLabelChange.ChangeLabels(nil, []string{"bug"})
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	// bug with comments editions
 	bugWithCommentEditions, createOp, err := repo.NewBug("bug with comments editions", "new bug")
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	createOpHash, err := createOp.Hash()
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	_, err = bugWithCommentEditions.EditComment(createOpHash, "first comment edited")
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	commentOp, err := bugWithCommentEditions.AddComment("first comment")
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	commentOpHash, err := commentOp.Hash()
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	_, err = bugWithCommentEditions.EditComment(commentOpHash, "first comment edited")
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	// bug status changed
 	bugStatusChanged, _, err := repo.NewBug("bug status changed", "new bug")
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	_, err = bugStatusChanged.Close()
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	_, err = bugStatusChanged.Open()
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	// bug title changed
 	bugTitleEdited, _, err := repo.NewBug("bug title edited", "new bug")
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	_, err = bugTitleEdited.SetTitle("bug title edited again")
-	if err != nil {
-		return nil, err
-	}
+	require.NoError(t, err)
 
 	return []*testCase{
 		&testCase{
@@ -157,8 +121,7 @@ func testCases(repo *cache.RepoCache, identity *cache.IdentityCache) ([]*testCas
 			bug:     bugTitleEdited,
 			numOrOp: 2,
 		},
-	}, nil
-
+	}
 }
 
 func TestPushPull(t *testing.T) {
@@ -188,8 +151,7 @@ func TestPushPull(t *testing.T) {
 	defer backend.Close()
 	interrupt.RegisterCleaner(backend.Close)
 
-	tests, err := testCases(backend, author)
-	require.NoError(t, err)
+	tests := testCases(t, backend, author)
 
 	// generate project name
 	projectName := generateRepoName()
@@ -224,7 +186,10 @@ func TestPushPull(t *testing.T) {
 	start := time.Now()
 
 	// export all bugs
-	for result := range exporter.ExportAll(backend, time.Time{}) {
+	events, err := exporter.ExportAll(backend, time.Time{})
+	require.NoError(t, err)
+
+	for result := range events {
 		require.NoError(t, result.Err)
 	}
 	require.NoError(t, err)
@@ -258,7 +223,7 @@ func TestPushPull(t *testing.T) {
 			// so number of operations should double
 			require.Len(t, tt.bug.Snapshot().Operations, tt.numOrOp*2)
 
-			// verify operation have correcte metadata
+			// verify operation have correct metadata
 			for _, op := range tt.bug.Snapshot().Operations {
 				// Check if the originals operations (*not* SetMetadata) are tagged properly
 				if _, ok := op.(*bug.SetMetadataOperation); !ok {
@@ -274,7 +239,7 @@ func TestPushPull(t *testing.T) {
 			bugGithubID, ok := tt.bug.Snapshot().GetCreateMetadata(keyGithubId)
 			require.True(t, ok)
 
-			// retrive bug from backendTwo
+			// retrieve bug from backendTwo
 			importedBug, err := backendTwo.ResolveBugCreateMetadata(keyGithubId, bugGithubID)
 			require.NoError(t, err)