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

fix: correct value for config path and robustify #130

Merged
merged 2 commits into from Sep 21, 2020

Conversation

lance
Copy link
Member

@lance lance commented Sep 18, 2020

The hardcoded, initial value for the configuration path was set to .faas/config. But configPath() immediately sets this to the correct value of ~/.config. Both the create and init commands use configPath() to search for additional templates, if they exist, and were each doing filepath.Join(configPath(), "faas", "templates"). This commit also changes configPath() so that it is ~/.config/faas and does so in a cross platform friendly way. If the $HOME directory cannot be determined, the config is assumed to be at ./.config/faas.

The hardcoded, initial value for the configuration path was set to
`.faas/config`. But `configPath()` immediately sets this to the correct
value of ~/.config. Both the create and init commands use `configPath()`
to search for additional templates, if they exist, and were each doing
`filepath.Join(configPath(), "faas", "templates")`. This commit also
changes `configPath()` so that it is `~/.config/faas` and does so in a
cross platform friendly way. If the `$HOME` directory cannot be
determined, the config is assumed to be at `./.config/faas`.
@lance lance requested a review from a team September 18, 2020 19:43
@lance lance self-assigned this Sep 18, 2020
Copy link
Member

@lkingland lkingland left a comment

Choose a reason for hiding this comment

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

Good catch, looks like the default initial value was not correctly being used in the function originally; probably a casualty of refactor. Nice to see the implementation updated to use filepath now too. That global variable serves no purpose now and can be removed to make it even cleaner.

cmd/root.go Outdated
@@ -11,7 +12,7 @@ import (
"github.com/boson-project/faas"
)

var config = "~/.faas/config" // Location of the optional system-wide config file.
var config = configPath() // Location of the optional system-wide config file.
Copy link
Member

Choose a reason for hiding this comment

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

This line can now be removed, as it's been correctly updated to be directly inlined into the configPath function using filepath to make it OS agnostic.

Indeed it should probably be removed as the configPath() static function should be used preferentially to a global variable.

Copy link
Contributor

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM

@lance lance merged commit fae27da into knative:main Sep 21, 2020
@lance lance deleted the tweak-config-path branch September 21, 2020 18:35
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.

None yet

3 participants