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

Remove default configuration and add fallback configuration #142

Merged
merged 2 commits into from
Oct 24, 2023

Conversation

samcoe
Copy link
Contributor

@samcoe samcoe commented Oct 24, 2023

This PR removes the knowledge of a default configuration from go-gh and replaces it with the ability to supply a fallback configuration to the config.Read function. This removes application level knowledge from go-gh and allows us to push it back into gh. One decision I made was that the fallback configuration would not be used unless both config.yml and hosts.yml were not found, previously the default configuration would be used whenever config.yml was not found. This change feels appropriate as we are now treating config.yml and hosts.yml as one single configuration and hides the details that they are actually two separate files from the caller.

⚠️ Note that this is a breaking change to the interface for config.Read which is exported, but in located in the config package which is only meant for use inside gh and has a warning about non-backwards compatible changes.

pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config_test.go Outdated Show resolved Hide resolved
pkg/config/config_test.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
@@ -89,7 +89,7 @@ func optionsNeedResolution(opts ClientOptions) bool {
}

func resolveOptions(opts ClientOptions) (ClientOptions, error) {
cfg, _ := config.Read()
cfg, _ := config.Read(nil)
Copy link
Member

Choose a reason for hiding this comment

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

It's kind of a shame with this pattern that we end up passing nil around everywhere. Oh well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Open to better suggestions if you have them.

Copy link
Member

Choose a reason for hiding this comment

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

The only reasonable one I have is using variadic functional opts. Everything else is a much bigger change than I think we would want to tackle.

@samcoe samcoe requested a review from williammartin October 24, 2023 09:30

if tt.fallback != nil {
// Assert that load returns an equivalent copy of fallvback.
assert.Equal(t, tt.fallback.entries.String(), cfg.entries.String())
Copy link
Member

Choose a reason for hiding this comment

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

Man, I totally get why the yaml Node has a slice for content now rather than a map, it's so it can always marshal in a stable order!

Copy link
Member

@williammartin williammartin left a comment

Choose a reason for hiding this comment

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

I'm not sure there's ever a good reason to use assert instead of require but I don't think that should block this PR!

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