fetches lists using Lists client wrapper #4787
Conversation
Current Aviator status
This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time. See the real-time status of this PR on the Aviator webapp. Use the Aviator Chrome Extension to see the status of your PR within GitHub.
|
src/internal/m365/backup_test.go
Outdated
| // but it should be more than one. | ||
| assert.Less(t, test.expected, len(collections)) | ||
| // but it should be more than zero. | ||
| assert.Less(t, 0, len(collections)) |
There was a problem hiding this comment.
NotEmpty is what you want here. Both for the nature of the check, and because a failure will show you what's in the collections.
| assert.Less(t, 0, len(collections)) | |
| assert.NotEmpty(t, collections) |
src/internal/m365/backup_test.go
Outdated
| assert.Equal(t, path.SharePointService.String(), collection.FullPath().Service().String()) | ||
| assert.Equal(t, path.ListsCategory.String(), collection.FullPath().Category().String()) |
There was a problem hiding this comment.
Service and category are comparable without stringifying.
| assert.Equal(t, path.SharePointService.String(), collection.FullPath().Service().String()) | |
| assert.Equal(t, path.ListsCategory.String(), collection.FullPath().Category().String()) | |
| assert.Equal(t, path.SharePointService, collection.FullPath().Service()) | |
| assert.Equal(t, path.ListsCategory, collection.FullPath().Category()) |
| var ( | ||
| metrics support.CollectionMetrics | ||
| el = errs.Local() | ||
| objects int64 |
There was a problem hiding this comment.
nit: avoid the term objects. Both because golang has no objects, and because it's too generic and easily misunderstood.
| objects int64 | |
| numLists int64 |
| var lists = []models.Listable{} | ||
| defer sc.finishPopulation(ctx, metrics) | ||
|
|
||
| // TODO: Insert correct ID for CollectionProgress |
There was a problem hiding this comment.
what ID are you referring to?
| atomic.AddInt64(&objects, 1) | ||
|
|
||
| if err := writer.WriteObjectValue("", entry); err != nil { | ||
| err = clues.Wrap(err, "writing list to serializer").Label(fault.LabelForceNoBackupCreation) |
There was a problem hiding this comment.
Can you please make sure that any error returned by a func that does not take in a ctx in its parameters adds the ctx to the error wrap? This helps ensure we get a complete set of insights into the error.
| err = clues.Wrap(err, "writing list to serializer").Label(fault.LabelForceNoBackupCreation) | |
| err = clues.WrapWC(ctx, err, "writing list to serializer").Label(fault.LabelForceNoBackupCreation) |
|
|
||
| size := int64(len(byteArray)) | ||
| go func(id string) { |
There was a problem hiding this comment.
While you're in this code, it'd be nice to have this gofunc moved from an ad-hoc func to a fully declared function. That would allow us to write unit tests around that func without having to engage in the full stream processing.
| metrics.Successes++ | ||
|
|
||
| rc := io.NopCloser(bytes.NewReader(entryBytes)) | ||
| itemID := ptr.Val(entry.GetId()) |
There was a problem hiding this comment.
isn't this the same as the id that the function takes in as a parameter?
|
|
||
| metrics.Successes++ | ||
| var ( | ||
| entry models.Listable |
There was a problem hiding this comment.
Nit: entry is a generic term and doesn't overlap with existing vocabulary, nor does it describe the item.
| entry models.Listable | |
| item models.Listable | |
| // alternatively: list |
| betaService := sc.betaService | ||
| if betaService == nil { | ||
| return metrics, clues.NewWC(ctx, "beta service required") | ||
| logger.Ctx(ctx).Error(clues.New("beta service required")) | ||
|
|
There was a problem hiding this comment.
no empty line needed on 2-line returns.
| // fullPath indicates the hierarchy within the collection | ||
| fullPath path.Path | ||
| // jobs contain the SharePoint.Site.ListIDs for the associated list(s). | ||
| // jobs contain the SharePoint.List.IDs or SharePoint.Page.IDs | ||
| jobs []string |
There was a problem hiding this comment.
I know it's not strictly part of your change, but we don't use the term jobs anywhere else in the repo. Can you change this to items?
|
Kudos, SonarCloud Quality Gate passed!
|
enables sharepoint to use lists backup handler for lists ops Changes previously approved in: - #4786 - #4787 - #4909 #### Does this PR need a docs update or release note? - [x] ⛔ No #### Type of change <!--- Please check the type of change your PR introduces: ---> - [x] 🌻 Feature #### Issue(s) #4754 #### Test Plan <!-- How will this be tested prior to merging.--> - [x] 💪 Manual - [x] ⚡ Unit test - [x] 💚 E2E
fetches lists and its relationships using Lists client wrapper
Does this PR need a docs update or release note?
Type of change
Issue(s)
#4754
Test Plan