Skip to content
This repository has been archived by the owner on Mar 15, 2024. It is now read-only.

Add missing db DSN flag and refactor code #7

Merged
merged 1 commit into from
Mar 2, 2023

Conversation

masih
Copy link
Member

@masih masih commented Mar 2, 2023

Add missing flag for db DSN so that database URL can be configured. Move the db config instantiation out to config. This reduces irrelevant code in the recorder constructor and makes the constructor easier to read.

Do not start the DB connection pool until the server is started and clean up the pool after shutdown.

Add missing validation tests that make use of the test data.

Improve logging by reducing duplicate properties and consistent terminology in messages. Reduce log level for messages that are likely to be verbose.

Add missing flag for db DSN so that database URL can be configured. Move
the db config instantiation out to config. This reduces irrelevant code
in the recorder constructor and makes the constructor easier to read.

Do not start the DB connection pool until the server is started and
clean up the pool after shutdown.

Add missing validation tests that make use of the test data.

Improve logging by reducing duplicate properties and consistent
terminology in messages. Reduce log level for messages that are likely
to be verbose.
@@ -14,27 +13,33 @@ import (
var logger = log.Logger("lassie/event_recorder/cmd")

func main() {
log.SetAllLoggers(log.LevelInfo)
ctx := context.Background()

// TODO: add flags for all options eventually.
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider using urfav.cli/v2 or similar

Copy link
Member Author

@masih masih Mar 2, 2023

Choose a reason for hiding this comment

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

The code doesn't aim to provide a CLI tool. Instead, it is a service that will almost always will be running as a long-running process. As such, the flags are no where near complex enough to introduce big dependencies like urfave.

Will revisit as the flags grow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I considered introducing urfave.cli/v2 as well, but I ultimately ended up at the same opinion of masih. The only reason there's a command is because it needs to be ran programatically. Not really made to be a CLI. If the options become way more configurable then something might be needed to help a user configure the command.

@masih masih merged commit ca29b07 into main Mar 2, 2023
@masih masih deleted the masih/fix_options_add_tests_and_logging branch March 2, 2023 11:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants