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

CBG3026 prereq: More low-risk cleanup/refactoring #7120

Merged
merged 2 commits into from
Sep 20, 2024
Merged

Conversation

bbrks
Copy link
Member

@bbrks bbrks commented Sep 20, 2024

This PR contains the changes unrelated to DatabaseInitManager that are in the upcoming CBG-3026 PR just to completely isolate those changes given the complexity.

  • Move DatabaseLogCtx earlier in _getOrAddDatabaseFromConfig so we can identify which database init-related logging is for
  • t.Run->rt.Run fixes (prevents panics with failing subtests)
  • Minor rename and TODO comment for upcoming work

Integration Tests

- Move `DatabaseLogCtx` earlier in `_getOrAddDatabaseFromConfig` so we can identify which database init-related logging is for
- `t.Run`->`rt.Run` fixes (prevents panics with failing subtests)
- Minor rename and TODO comment for upcoming work
torcolvin
torcolvin previously approved these changes Sep 20, 2024
@@ -588,6 +588,14 @@ func (sc *ServerContext) _getOrAddDatabaseFromConfig(ctx context.Context, config
dbName = spec.BucketName
}

// Generate database context options from config and server context
contextOptions, err := dbcOptionsFromConfig(ctx, sc, &config.DbConfig, dbName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you check that this is okay that this isn't going to return an error for db.ValidateDatabaseName?

Copy link
Member Author

@bbrks bbrks Sep 20, 2024

Choose a reason for hiding this comment

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

Ah! I missed that. I also realised this code is being put before the defer as well, so I'll move things down a bit. There's also the duplicate check we probably want to do first to exit early.

@bbrks bbrks self-assigned this Sep 20, 2024
@bbrks bbrks merged commit 03a3f3d into main Sep 20, 2024
38 checks passed
@bbrks bbrks deleted the CBG3026-prereq branch September 20, 2024 13:53
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