Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CBG-3843: include collection names in status of resync #6839

Merged
merged 3 commits into from
May 24, 2024
Merged

CBG-3843: include collection names in status of resync #6839

merged 3 commits into from
May 24, 2024

Conversation

gregns1
Copy link
Contributor

@gregns1 gregns1 commented May 22, 2024

CBG-3843

  • Refactored function that gets collection id's for resync to also return the collection names
  • Add the list of collection names to the manager object to then be used in the status function
  • Change has been made to both DCP resync and query resync
  • Had to make the collection list on status struct omitempty as when calling endpoint to start resync the status is returned but at this point the Run() function hasn't started to calculate the collections its running on. To avoid the initial POST call returning null for collections I have made it omit empty

Pre-review checklist

  • Removed debug logging (fmt.Print, log.Print, ...)
  • Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
  • Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Integration Tests

@gregns1 gregns1 self-assigned this May 22, 2024
@gregns1 gregns1 requested a review from bbrks May 24, 2024 13:30
@gregns1 gregns1 assigned bbrks and unassigned gregns1 May 24, 2024
Copy link
Member

@bbrks bbrks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a suggestion on using an alternative assertion facility

Comment on lines 2674 to 2675
// RequireCollections Iterates through two lists of collections and asserts they both contain the same collections
func RequireCollections(t testing.TB, collectionNamesFromStatus, collectionSpecified []string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to use (assert|require).ElementsMatch for an equivalent to this

https://pkg.go.dev/github.com/stretchr/testify/assert#ElementsMatch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had no idea that existed! Updated to use that assertion.

@bbrks bbrks merged commit 99d3d6d into main May 24, 2024
34 checks passed
@bbrks bbrks deleted the CBG-3843 branch May 24, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants