hookdeck-cli icon indicating copy to clipboard operation
hookdeck-cli copied to clipboard

refactor: Config parsing & precedence

Open alexluong opened this issue 5 months ago • 14 comments

This is a fairly impactful PR as it involves a breaking change as well as a lot of code changes related to pkg/config which impacts every command. Some key pointers:

Global & local config

Previously, we maintain 2 config scope: global & local config. Local config can be empty. This is the precedence for config values:

  • accept value from flag
  • read from local
  • read from global

Because of this, we can also save things in the local config to override the global config. For example, $ hookdeck project use --local will store workspace_id value in the local config so that future commands in this scope will use the selected project. In this case, the local config file could be as short as:

workspace_id = "tm_123"

Changes

This PR removes the concept of global & local config. There's only 1 singular config. If there's a config within the local scope ($PWD/.hookdeck/config.toml) or if the user provides the --config /path/to/config.toml flag, then the CLI will use this config file only and ignore the global config file should one exist.

Test & refactor

Besides code to make the changes in the section above, I also refactored pkg/config with 2 main concerns:

  1. Make the code more testable (for example, using ConfigFS interface to mock different filesystem scenarios)
  2. Reduce public API for the package to the minimum

With that, I also added quite a bit of tests to ensure things behave correctly. Some main shoutouts:

  • TestGetConfigPath will test that the CLI will choose the correct config file from different user input / filesystem scenarios (whether config flag is passed, whether a local config exists, etc)

  • TestInitConfig will test what the final config looks like with different config files. You can see different scenarios in pkg/config/testdata. There's a README that gives some explanation for each scenario. Please feel free to add more if there's anything you'd like to test.

  • [ ] Add more config scenarios. Something I haven't considered: backward compatibility.

  • [ ] Should we consider adding config versioning moving forward?

  • [ ] The tests currently place a very strong emphasis on profile. There's no test for other config cases like LogLevel, Base URLs, etc. Is there anything you'd like to test more? Feel free to add your own if you'd like, or comment here.

alexluong avatar Sep 03 '24 00:09 alexluong