refactor: Config parsing & precedence #103

alexluong · 2024-09-03T00:57:00Z

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:

Make the code more testable (for example, using ConfigFS interface to mock different filesystem scenarios)
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 · 2024-09-03T00:58:31Z

pkg/cmd/project_use.go

+	// With the change in config management (either local or global, not both), this flag is no longer needed
+	// TODO: consider remove / deprecate
+	// lc.cmd.Flags().BoolVar(&lc.local, "local", false, "Pin active project to the current directory")


Wanna place some emphasis here given the change from this PR makes this flag obsolete. There's nothing we can do whether the user passes --local flag to $ hookdeck project use --local command or not.

@alexluong how does a user create a local config without this?

I don't think there's a way right now. The original thought process was people may write the config manually. We can certainly support a local flag for the login command maybe?

alexluong · 2024-09-03T01:04:03Z

pkg/config/config_test.go

+	// TODO: Consider this case. This is a breaking change.
+	// BREAKINGCHANGE
+	t.Run("local workspace only", func(t *testing.T) {
+		t.Parallel()
+
+		c := Config{
+			LogLevel:       "info",
+			ConfigFileFlag: "./testdata/local-workspace-only.toml",
+		}
+		c.InitConfig()
+
+		assert.Equal(t, "default", c.Profile.Name)
+		assert.Equal(t, "", c.Profile.APIKey)
+		assert.Equal(t, "local_workspace_id", c.Profile.TeamID)
+		assert.Equal(t, "", c.Profile.TeamMode)
+	})


This is the case I mentioned earlier.

workspace_id = "tm_123"

If anyone is in this situation (which there could very well be none), whenever they run any command, we'll ask them to login and after that they can use the CLI normally. However, when they go to another directory without this local config, we'll use the global config again.

alexluong · 2024-09-03T01:05:34Z

pkg/config/config_test.go

+	t.Run("remove profile multiple times", func(t *testing.T) {
+		t.Parallel()
+
+		// Arrange
+		c := Config{LogLevel: "info"}
+		c.ConfigFileFlag = setupTempConfig(t, "./testdata/multiple-profiles.toml")
+		c.InitConfig()
+
+		// Act
+		err := c.Profile.RemoveProfile()
+
+		// Assert
+		assert.NoError(t, err)
+		contentBytes, _ := ioutil.ReadFile(c.viper.ConfigFileUsed())
+		assert.NotContains(t, string(contentBytes), "account_2", `default profile "account_2" should be cleared`)
+		assert.NotContains(t, string(contentBytes), `profile =`, `profile key should be cleared`)
+
+		// Remove profile again
+
+		c2 := Config{LogLevel: "info"}
+		c2.ConfigFileFlag = c.ConfigFileFlag
+		c2.InitConfig()
+		err = c2.Profile.RemoveProfile()
+
+		contentBytes, _ = ioutil.ReadFile(c2.viper.ConfigFileUsed())
+		assert.NoError(t, err)
+		assert.NotContains(t, string(contentBytes), "[default]", `default profile "default" should be cleared`)
+		assert.NotContains(t, string(contentBytes), `api_key = "test_api_key"`, `default profile "default" should be cleared`)
+
+		// Now even though there are some profiles (account_1, account_3), when reading config
+		// we won't register any profile.
+		// TODO: Consider this case. It's not great UX. This may be an edge case only power users run into
+		// given it requires users to be using multiple profiles.
+
+		c3 := Config{LogLevel: "info"}
+		c3.ConfigFileFlag = c.ConfigFileFlag
+		c3.InitConfig()
+		assert.Equal(t, "default", c3.Profile.Name, `profile should be "default"`)
+		assert.Equal(t, "", c3.Profile.APIKey, "api key should be empty even though there are other profiles")
+	})


This is an interesting case where the user may have multiple profiles but we won't be able to pick up the existing ones and they'll have to login again.

I'm just wondering if this scenario I set up here is clear from your POV. Happy to elaborate further if it's unclear.

Do you think we should do anything here to improve the DX?

leggetter · 2024-09-03T06:41:08Z

It would be good to understand why we're considering this change to help discussion and also to feed into the CHANGELOG.

alexluong · 2024-09-03T09:14:28Z

@leggetter Hey, welcome back! For this, I think @alexbouchardd may be able to shed some more light on it too. From my POV, the key thing is around some potential confusion with the "local override global" model where it could be difficult to understand why the CLI behaves the way it does.

For example, let's say you have a local config $PWD/.hookdeck/config.toml like so

profile = "my_profile"

then you run $ hookdeck login -p another_profile. You can log in correctly, potentially to a new Hookdeck account, but when you run $ hookdeck whoami, it will be using "my_profile" again.

My example could be a bit contrived but it demonstrates the idea that it's difficult for both us (CLI) and end users to manage the relationship between global and local config files and what config should go where.

Using this new mental model, it's a lot clearer how things work.

A similar example:

$ hookdeck project use --local
# change workspace_id in $PWD/.hookdeck/config.toml
# let's say we change to PROJECT_A

# later (next day, next week, next year, etc)
# choose a different project, let's say PROJECT_B
$ hookdeck project use

$ hookdeck whoami

Using profile PROJECT_A .....
# even though we literally just select PROJECT_B

With that in mind, I can see sometimes it could be helpful to "override" the global config. The first thought is to have something similar to .vscode/settings.json model, for example, where it behaves like Lodash's merge(globalConfig, localConfig). That way, we can have some global config like log-level = "info" and depending on the project we can change the log-level accordingly.

Ultimately, there's pros and cons and we just have to figure out which model best suits our needs. Given the CLI doesn't have a ton of configuration and is mostly around profile management, I do think being more explicit right now to make things more deterministic could be a good option and we can attempt the override model at a later time when things make sense.

alexluong · 2024-09-04T01:36:54Z

@leggetter hey so I've been thinking and I'm curious what you think about this.

There's this use case that I'm imagining but I don't know if it's something worth supporting / considering.

Say there's an engineering organization using Hookdeck. They may have multiple different Hookdeck projects for different workflows. They may also separate environments (dev, staging, production) via projects, or even Hookdeck organization itself.

So, as a developer of said organization, I may want to do multiple things:

in project A repo, I'd like to run $ hookdeck listen 3000 to proxy events related to this project
navigate to project B repo, I'd like to run $ hookdeck listen 4000 to proxy events related to this project

Or even more complicated use case, in a monorepo with both project A & B, I'd like to run some arbitrary command to proxy events for both A and B to their corresponding ports of 3000 and 4000. Say make dev or something and the whole dev environment is up.

I wonder if we'd like to spend time now trying to come up with solutions for this, or if it's a future conversation? I would imagine this has an impact on the config file setup.

I remembered taking some inspiration from the Doppler CLI where they need to solve this problem. They have some sort of "profile - directory" mapping within their global config file. It's an interesting solution. I didn't love their CLI as a user but I can understand the complexity.

Ultimately, I'm just taking a step back and re-evaluating what problem the local $PWD/.hookdeck/config.toml solves. Maybe we (or mainly I) may have lost the plot a bit where we discuss the details instead of relating to a use case or problem? 😅

leggetter · 2024-09-09T10:30:02Z

@alexluong - I think the approach we're taking here, making global and local config mutually exclusive, is the right approach 👍 It means the scenarios you cover are still possible and avoids any unexpected side effects by global config being inherited, especially when there are very few things to configure!

I think that the fact we support different config files and profiles within the same config file is potentially powerful, but I wonder if anybody uses it right now. Food for thought, and no action is required on this at present.

I like CLIs (and other tools) that dynamically pick up local config and use it (e.g. detects a hookdeck.config.toml and uses it). So, maybe that's an approach we take in the future.

I don't think we need to discuss anything further in this PR. However, there's likely a few things to work out in #102

pkg/config/profile.go

alexluong · 2024-09-09T11:15:10Z

I like CLIs (and other tools) that dynamically pick up local config and use it (e.g. detects a hookdeck.config.toml and uses it). So, maybe that's an approach we take in the future.

We already do this. It automatically picks up .hookdeck/config.toml locally from where you run the CLI.

leggetter · 2024-09-09T11:16:05Z

I like CLIs (and other tools) that dynamically pick up local config and use it (e.g. detects a hookdeck.config.toml and uses it). So, maybe that's an approach we take in the future.

We already do this. It automatically picks up .hookdeck/config.toml locally from where you run the CLI.

Oh. Interesting!

alexluong · 2024-09-09T11:16:18Z

I don't think we need to discuss anything further in this PR. However, there's likely a few things to work out in #102

Totally, I just wanted to pause to make sure the approach is right. This PR is against #102 branch anyway, so I'll open more PRs to continue implementing everything mentioned in that description.

leggetter · 2024-09-19T04:40:28Z

@alexluong and additional thought on supporting the commuting of config to source control - if we want to support that then no co credentials should be within the config file.

alexluong · 2024-09-19T17:48:07Z

@leggetter I think the main thing is that we don't really use the config much besides credentials right now, so I don't think it's recommended to committing config files. If we do have more extensible configuration in the future, it may make sense to separate config vs credentials?

leggetter · 2024-09-19T17:49:38Z

@leggetter I think the main thing is that we don't really use the config much besides credentials right now, so I don't think it's recommended to committing config files. If we do have more extensible configuration in the future, it may make sense to separate config vs credentials?

I'd love to have local project selection and to be able to check that in.

alexluong · 2024-09-30T16:10:24Z

I'd love to have local project selection and to be able to check that in.

So should we separate the config into "config" and "profiles" where profile will have secrets and config are configuration that can be checked in to version control?

alexbouchardd · 2024-10-02T13:13:59Z

@alexluong I think we are going out of scope here. We are just trying to fix the precedence. Being able to check in the config is a good idea but I reckon best to open a different issue?

chore: add multi-source example

alexluong · 2024-10-03T14:39:47Z

@alexbouchardd yeah that's fair. In that case, any input on the PR and how we should proceed here? cc @leggetter

leggetter · 2024-10-04T14:39:38Z

@alexluong Ok, I agree splitting out the config can be out of scope for this issue.

But I think we should bring the --local config back and apply it to login and project use. Otherwise, we remove it here and then bring it back in another PR in the very near future.

but not both

alexluong · 2024-10-15T08:23:33Z

Hey @leggetter, I've been playing around with the idea of bringing the --local config back as you suggested.

I think it may not make sense for project use because you need the full config to work, so it has to start with login command. Otherwise, after you run hookdeck project use --local, we must copy the current credentials and store it in the local config file. That could work but it may be unexpected and lead to potential API keys leakage on GitHub.

As for login, it's a bit tricky. First, just to explain the flow of how the CLI works:

1: user runs command (say hookdeck login)
2: CLI parses global flags into Config struct
3: Config.InitConfig will run, using the flags to determine local vs global config file
4: CLI parses command-specific flags
5: command logic

So, if we want to support login --local flag, we will need a mechanism to change the config file used in step 5, after the program has already parses the original config file in step 3. It's doable. The only potential concern is what if the user is running in an environment where they don't have write permission in the global config path. The program will fail, even though it won't need that permission. But this is an edge case, so we can potentially acknowledge and move on?

There's already a way to login and save to a local config by specifying the config file. So essentially:

hookdeck login --local will be equivalent to hookdeck login --config ./.hookdeck/config.toml

Obviously this is subpar DX and should not be the final solution. Just curious what you think our next step should be regarding this flow.

leggetter · 2024-11-28T19:21:54Z

@alexluong - if we merge this, what does it resolve within #102 ?

alexluong · 2024-12-02T13:30:17Z

@leggetter soo it seems this PR's scope is not documented in #102. I think #102 requirements were part of a discussion with @alexbouchardd where we discussed some functional or behavior points. This PR is a more "internal" change where technically it doesn't affect how it's used for most users.

This PR changes how we parse config from "merging local & global" into "either local or global".

It is a breaking change, along with several other breaking changes from #102, so I figured it should be done together to avoid multiple breaking changes at once.

alexluong commented Sep 3, 2024

View reviewed changes

leggetter reviewed Sep 9, 2024

View reviewed changes

pkg/config/profile.go Show resolved Hide resolved

leggetter added 3 commits October 2, 2024 21:57

chore: add multi-source example

5de450d

chore: fix typo

45a7622

Merge pull request #104 from hookdeck/chore/multi-source-readme

7d33389

chore: add multi-source example

alexluong added 8 commits October 15, 2024 15:02

feat: Use either global or local config file

34d9c8b

but not both

chore: Remove debug log

b7dc5df

refactor: fs interface

9496ab4

test: getConfigPath

8defd13

test: improve test semantic using expected path variable

827435b

test: Parse config with different mock config scenarios

23767be

refactor: Clean config files & reduce package's public api surface

5006f6c

test: Methods related to config updates

d5e33d0

alexluong force-pushed the refactor-config branch from 9e805c0 to d5e33d0 Compare October 15, 2024 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Config parsing & precedence #103

refactor: Config parsing & precedence #103

alexluong commented Sep 3, 2024 •

edited

Loading

alexluong Sep 3, 2024

leggetter Sep 19, 2024

alexluong Sep 30, 2024

alexluong Sep 3, 2024 •

edited

Loading

alexluong Sep 3, 2024

leggetter commented Sep 3, 2024 •

edited

Loading

alexluong commented Sep 3, 2024

alexluong commented Sep 4, 2024

leggetter commented Sep 9, 2024

alexluong commented Sep 9, 2024

leggetter commented Sep 9, 2024

alexluong commented Sep 9, 2024

leggetter commented Sep 19, 2024

alexluong commented Sep 19, 2024

leggetter commented Sep 19, 2024

alexluong commented Sep 30, 2024

alexbouchardd commented Oct 2, 2024

alexluong commented Oct 3, 2024

leggetter commented Oct 4, 2024

alexluong commented Oct 15, 2024

leggetter commented Nov 28, 2024

alexluong commented Dec 2, 2024

refactor: Config parsing & precedence #103

Are you sure you want to change the base?

refactor: Config parsing & precedence #103

Conversation

alexluong commented Sep 3, 2024 • edited Loading

Global & local config

Changes

Test & refactor

alexluong Sep 3, 2024

Choose a reason for hiding this comment

leggetter Sep 19, 2024

Choose a reason for hiding this comment

alexluong Sep 30, 2024

Choose a reason for hiding this comment

alexluong Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

alexluong Sep 3, 2024

Choose a reason for hiding this comment

leggetter commented Sep 3, 2024 • edited Loading

alexluong commented Sep 3, 2024

alexluong commented Sep 4, 2024

leggetter commented Sep 9, 2024

alexluong commented Sep 9, 2024

leggetter commented Sep 9, 2024

alexluong commented Sep 9, 2024

leggetter commented Sep 19, 2024

alexluong commented Sep 19, 2024

leggetter commented Sep 19, 2024

alexluong commented Sep 30, 2024

alexbouchardd commented Oct 2, 2024

alexluong commented Oct 3, 2024

leggetter commented Oct 4, 2024

alexluong commented Oct 15, 2024

leggetter commented Nov 28, 2024

alexluong commented Dec 2, 2024

alexluong commented Sep 3, 2024 •

edited

Loading

alexluong Sep 3, 2024 •

edited

Loading

leggetter commented Sep 3, 2024 •

edited

Loading