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

Introduce environment variable for custom config dir #291

Merged
merged 2 commits into from
Nov 3, 2020

Conversation

unguiculus
Copy link
Member

Signed-off-by: Reinhard Nägele [email protected]

@torstenwalter
Copy link
Contributor

For me this looks good!

if util.FileExists(filePath) {
return filePath, nil
return cfgFile, nil
Copy link
Member

Choose a reason for hiding this comment

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

This is always an empty string here, right? I'm sure I'm reading it wrong, but where is cfgFile set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. I improved that and also added a test.

@@ -17,7 +17,7 @@ package config
import (
"fmt"
"os"
"path"
"path/filepath"
Copy link
Member

Choose a reason for hiding this comment

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

Just being clear, we're changing this why? looking it up, is the purpose for platform compatibility in general? Does this belong in the same PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point and I probably was just to lazy to factor that out. I notice that usage of path vs. filepath is inconsistent and just fixed it along the way. And yes, the reason is platform compatibility. I'm gonna create a separate PR for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created #292

Copy link
Member Author

Choose a reason for hiding this comment

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

I rebased this PR on top of #292. So, #292 should be merged first.

Copy link
Member Author

Choose a reason for hiding this comment

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

I rebased now after the merge.

@@ -88,8 +88,12 @@ func LoadConfiguration(cfgFile string, cmd *cobra.Command, printConfig bool) (*C
v.SetConfigFile(cfgFile)
} else {
v.SetConfigName("ct")
for _, searchLocation := range configSearchLocations {
v.AddConfigPath(searchLocation)
if cfgFile, ok := os.LookupEnv("CT_CONFIG_DIR"); ok {
Copy link
Member

Choose a reason for hiding this comment

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

I get the use case here, but just checking on implementation. We have --config flag now, not --config-dir. Why not keep it consistent with CT_CONFIG path to file rather than dir?

Copy link
Member Author

Choose a reason for hiding this comment

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

I probalby should have explained that better. CT_CONFIG_DIR overrides the default locations where config files are searched. This is not only about the config file for ct but also those for Yamale and yamllint. This will be required when we install ct into a cache location in #58, so config files are still found.

See https://github.com/helm/chart-testing-action/pull/58/files#diff-c141db7e865976e07d678c49af2c6288b4c97d4a0af34ac25905bd46dde8301aR74

Copy link
Member

@scottrigby scottrigby left a comment

Choose a reason for hiding this comment

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

💯

@unguiculus unguiculus merged commit 2a4dfd4 into helm:master Nov 3, 2020
@unguiculus unguiculus deleted the feature/config branch November 3, 2020 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants