Skip to content

Conversation

@jradtilbrook
Copy link
Contributor

@jradtilbrook jradtilbrook commented May 8, 2024

This started out to see how we could better manage the configuration for the CLI a little nicer in some sort of hierarchy and ended up quite big. I think where its landed is quite ideal though.

With this change, the configuration follows similar to other Unix configuration files where you have a local config (current directory), a user config (somewhere under $HOME), and potentially down the track a system config (under /etc).

Configuration precedence follows that order as well. So the configuration file "closest" to where bk is invoked has the highest precedence.

This will mean we can remove the LocalConfig object (created another card already).

The file structure was not changed. But access patterns have been improved.

This has also resulted in much less code with more deletions than additions (and that doesn't factor in the followup to remove LocalConfig).

@jradtilbrook jradtilbrook force-pushed the sup-2138-re-evaluate-config-architecture branch from fa0cc02 to b92f867 Compare May 8, 2024 12:36
@jradtilbrook
Copy link
Contributor Author

Tests are failing locally for me. So we appear to have a flaky test. I believe it has something to do with gock as it mentions its not really meant for current use (in our parallel tests).

I'll spend some time to try fix it, but at the least, I'll remove the parallel testing if it takes too long

@jradtilbrook jradtilbrook marked this pull request as ready for review May 9, 2024 00:29
lizrabuya
lizrabuya previously approved these changes May 9, 2024
Copy link
Contributor

@PriyaSudip PriyaSudip left a comment

Choose a reason for hiding this comment

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

Reviewing

PriyaSudip
PriyaSudip previously approved these changes May 9, 2024
@jradtilbrook jradtilbrook dismissed stale reviews from PriyaSudip and lizrabuya via 1842f2c May 9, 2024 01:37
@jradtilbrook
Copy link
Contributor Author

I've opted to remove gock and go with a stubbed RoundTripper for the tests instead. That allows them to work concurrently

@jradtilbrook jradtilbrook enabled auto-merge (squash) May 9, 2024 01:42
@jradtilbrook jradtilbrook merged commit 9c106ac into 3.x May 9, 2024
@jradtilbrook jradtilbrook deleted the sup-2138-re-evaluate-config-architecture branch May 9, 2024 02:39
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.

3 participants