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

feat: config init #2147

Merged
merged 26 commits into from
Sep 25, 2023
Merged

feat: config init #2147

merged 26 commits into from
Sep 25, 2023

Conversation

markphelps
Copy link
Collaborator

@markphelps markphelps commented Sep 18, 2023

Fixes: FLI-600
exploration to add a new command/subcommand:

» ./bin/flipt config init
? Configuration file path: /Users/markphelps/Library/Application Support/flipt/config.yml
» ./bin/flipt

    _________       __
   / ____/ (_)___  / /_
  / /_  / / / __ \/ __/
 / __/ / / / /_/ / /_
/_/   /_/_/ .___/\__/
         /_/

Version: dev
Commit: 02e90e36f4c74bd9b134ac377ebd06548f9640bf
Build Date: 2023-09-20T15:35:57Z
Go Version: go1.21.0
OS/Arch: darwin/arm64


API: http://0.0.0.0:8080/api/v1
UI: http://0.0.0.0:8080

^C2023-09-20T11:37:02-04:00	INFO	shutting down...
2023-09-20T11:37:02-04:00	INFO	shutting down HTTP server...	{"server": "http"}
2023-09-20T11:37:02-04:00	INFO	shutting down GRPC server...	{"server": "grpc"}

» ./bin/flipt config init
? Configuration file path: /Users/markphelps/Library/Application Support/flipt/config.yml
? File exists. Overwrite? Yes

This will write a default config.yml file to the user's home config directory. Mostly to enable getting up and running quickly with local Flipt


Here is the config it generates:

# yaml-language-server: $schema=https://raw.githubusercontent.com/flipt-io/flipt/main/config/flipt.schema.json

version: "1.0"
log:
  level: INFO
  encoding: console
  grpc_level: ERROR
ui:
  default_theme: system
cors:
  enabled: false
  allowed_origins:
    - '*'
server:
  host: 0.0.0.0
  http_port: 8080
  https_port: 443
  grpc_port: 9000
storage:
  type: database
db:
  url: file:/Users/markphelps/Library/Application Support/flipt/flipt.db
  max_idle_conn: 2
  prepared_statements_enabled: true
meta:
  check_for_updates: true
  telemetry_enabled: true

Also

  • Finally removes deprecated config options older than (deprecated since) 6+ months
  • Changes all command errors (atleast initial errors) to go to stderr instead of using the zap logger / fatal, which logs them to stdout at ERROR level, reasoning is below

On Errors

Going forward, I think we should adopt the position that there are two types of errors:

  • Errors that stop a command from starting to run, like invalid arguments (ie: flipt foo). These errors should be logged to STDERR without logging/formatting
  • Errors that occur after a command has started, ie runtime errors. I think these errors should be logged using our zap logger which is configured by the user, and logged at ERROR log level.

cmd/flipt/config.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Merging #2147 (940da83) into main (2f3f66d) will decrease coverage by 0.07%.
Report is 1 commits behind head on main.
The diff coverage is 44.00%.

@@            Coverage Diff             @@
##             main    #2147      +/-   ##
==========================================
- Coverage   70.25%   70.18%   -0.07%     
==========================================
  Files          76       76              
  Lines        7167     7151      -16     
==========================================
- Hits         5035     5019      -16     
+ Misses       1837     1834       -3     
- Partials      295      298       +3     
Files Changed Coverage Δ
internal/cmd/http.go 3.10% <0.00%> (+0.07%) ⬆️
internal/config/config.go 87.16% <0.00%> (-0.04%) ⬇️
internal/config/cors.go 100.00% <ø> (ø)
internal/config/database.go 91.66% <ø> (ø)
internal/config/deprecations.go 57.14% <ø> (-42.86%) ⬇️
internal/config/experimental.go 100.00% <ø> (ø)
internal/config/meta.go 100.00% <ø> (ø)
internal/config/server.go 93.75% <0.00%> (-6.25%) ⬇️
internal/config/storage.go 81.69% <ø> (ø)
internal/config/ui.go 100.00% <ø> (ø)
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@markphelps markphelps added the needs docs Requires documentation updates label Sep 20, 2023
@markphelps markphelps changed the title feat(wip): config init feat: config init Sep 20, 2023
@markphelps markphelps marked this pull request as ready for review September 20, 2023 15:38
@markphelps markphelps requested a review from a team as a code owner September 20, 2023 15:38
@markphelps markphelps marked this pull request as draft September 21, 2023 15:22
@markphelps markphelps marked this pull request as ready for review September 21, 2023 15:34
@markphelps markphelps marked this pull request as draft September 21, 2023 23:26
config/flipt.schema.json Outdated Show resolved Hide resolved
@yquansah
Copy link
Contributor

I wonder if we do need all the Redis cache stuff written out to the configuration file if cache is turned off by default

@markphelps
Copy link
Collaborator Author

I wonder if we do need all the Redis cache stuff written out to the configuration file if cache is turned off by default

yeah that's a good point, would require us to implement custom MarshalJSON and MarshalYAML methods for the CacheConfig struct, but that's prob not a bad idea. The same could be said for tracing as well.

We should prob come up with a rubric of where to draw the line as in what gets written to the default config and what doesn't.

Like should authentication and audit also be written to the default config? even though they are not enabled by default either? But to enable them you have to enable one of their sub properties (methods/sinks)

@markphelps
Copy link
Collaborator Author

I wonder if we do need all the Redis cache stuff written out to the configuration file if cache is turned off by default

yeah that's a good point, would require us to implement custom MarshalJSON and MarshalYAML methods for the CacheConfig struct, but that's prob not a bad idea. The same could be said for tracing as well.

We should prob come up with a rubric of where to draw the line as in what gets written to the default config and what doesn't.

Like should authentication and audit also be written to the default config? even though they are not enabled by default either? But to enable them you have to enable one of their sub properties (methods/sinks)

@yquansah checkout the updated output in the PR description to see what I got it down to

@markphelps markphelps marked this pull request as ready for review September 22, 2023 18:36
Copy link
Contributor

@yquansah yquansah left a comment

Choose a reason for hiding this comment

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

@markphelps This looks good to me!

@markphelps markphelps enabled auto-merge (squash) September 25, 2023 13:20
@markphelps markphelps merged commit be807aa into main Sep 25, 2023
22 of 25 checks passed
@markphelps markphelps deleted the mp/config-init branch September 25, 2023 14:03
@markphelps markphelps removed the needs docs Requires documentation updates label Oct 2, 2023
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