From 93e731fd0d365c0cf49dbb7aea371e48f46e1f11 Mon Sep 17 00:00:00 2001 From: Amine Date: Fri, 5 Jul 2019 18:32:51 +0200 Subject: [bridge/github] improve comments and documentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [bridge/github] improve error handling and tests Co-Authored-By: Michael Muré --- bridge/core/bridge.go | 2 +- bridge/core/interfaces.go | 2 +- bridge/github/export.go | 77 ++++++++++++++++++++------------------ bridge/github/export_test.go | 89 ++++++++++++++------------------------------ 4 files changed, 69 insertions(+), 101 deletions(-) diff --git a/bridge/core/bridge.go b/bridge/core/bridge.go index 30e051be..d04e88e4 100644 --- a/bridge/core/bridge.go +++ b/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) } diff --git a/bridge/core/interfaces.go b/bridge/core/interfaces.go index 5cc7006a..76d66fb4 100644 --- a/bridge/core/interfaces.go +++ b/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) } diff --git a/bridge/github/export.go b/bridge/github/export.go index 40044ca1..5e5b1a21 100644 --- a/bridge/github/export.go +++ b/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{ diff --git a/bridge/github/export_test.go b/bridge/github/export_test.go index 827152b7..80660e77 100644 --- a/bridge/github/export_test.go +++ b/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) -- cgit