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.

Sign up for GitHub

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

Jump to bottom

CBG-4336: add updated at field for persisted configs #7265

Merged
merged 7 commits into from
Jan 9, 2025
Merged

CBG-4336: add updated at field for persisted configs #7265

merged 7 commits into from
Jan 9, 2025

Conversation

gregns1
Copy link
Contributor

@gregns1 gregns1 commented Jan 7, 2025 •
edited
Loading

CBG-4336

  • Add updated at timestamp to persisted configs
  • The following configs are included:
    • Registry
    • Db configs
    • Replication configs
    • User and role docs

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 Jan 7, 2025
@bbrks
Copy link
Member

bbrks commented Jan 7, 2025 •
edited
Loading

@JRascagneres Can you review and make sure the updated_at name and format/UTC aligns with stuff on your side? And input on whether created_at is necessary as well.

@bbrks bbrks requested review from JRascagneres and bbrks January 7, 2025 15:03
@bbrks
Copy link
Member

bbrks commented Jan 7, 2025

As discussed - add a created_at field as well, but don't care about timestamp format - keep that as-is.

@JRascagneres JRascagneres removed their request for review January 7, 2025 19:30
@gregns1 gregns1 assigned bbrks and unassigned gregns1 Jan 9, 2025
bbrks
bbrks requested changes Jan 9, 2025
View reviewed changes
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, wonder if we can simplify a bit around Principal with the SetCreatedAt usage - but nitpicking.

auth/auth.go Outdated
@@ -631,6 +631,9 @@ func (auth *Authenticator) UpdateUserEmail(u User, email string) error {
if err != nil {
return nil, err
}
currentUser.SetUpdatedAt()
currentUser.SetCreatedAt(currentUser.GetCreatedAt())
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why the Set(Get()) pattern is required. I think it's not nescessary to call SetCreatedAt in these places at all, since the time is already set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, not sure why I thought this was needed. Will remove this now

@gregns1 gregns1 enabled auto-merge (squash) January 9, 2025 12:17
@gregns1 gregns1 merged commit 44ccb52 into main Jan 9, 2025
37 of 38 checks passed
@gregns1 gregns1 deleted the CBG-4336 branch January 9, 2025 12:21
gregns1 added a commit that referenced this pull request Jan 9, 2025
@gregns1
CBG-4336: add updated at field for persisted configs (#7265)
49fa6a7 
* CBG-4336: add updated at field for persisted configs

* change time to utc

* fix default collection test issues

* flakyness with tests

* add delay to other tests too

* updates to add created_at field to configs

* remove unnecessary method
@gregns1 gregns1 mentioned this pull request Jan 9, 2025
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbrks bbrks bbrks approved these changes

Assignees

@bbrks bbrks

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gregns1 @bbrks