Ordered helmfile(s) from a directory #160
Conversation
mumoshu
force-pushed
the
ordered-helmfiles-from-dir
branch
from
986533d
to
9a9a30b
Compare
main.go
Outdated
| dir = DefaultHelmfileDirectory | ||
| } | ||
|
|
||
| if dir != "" { |
There was a problem hiding this comment.
I think this is clearer
if dir != "" {
...
else if fileExists(DefaultHelmfileDirectory) && directoryExists(DefaultHelmfileDirectory) {
...
}
There was a problem hiding this comment.
What would be included in if and else if bodies respectively? We need dir to be set for use in https://github.com/roboll/helmfile/pull/160/files#diff-7ddfb3e035b42cd70649cc33393fe32cR325.
There was a problem hiding this comment.
Would it look clearer now?
main.go
Outdated
| return files, nil | ||
| } | ||
|
|
||
| if fileExists(DefaultHelmfile) { |
There was a problem hiding this comment.
Should we complain if we have both a helmfile dir and a helmfile? Or should we load both? Currently it would ignore the helmfile and use the helmdir.
There was a problem hiding this comment.
That's a sneaky kind of behavior, so whatever we do, I hope we make it explicit and/or noisy. Maybe a warning + using the helmdir? Or an error and immediate exit if we want to really prevent confusion.
There was a problem hiding this comment.
Typically with apps that have a conf.d structure, they also support a main config file. That main config file then will then have an option to point to a conf.d folder. For example systemd. Now, I'm not advocating adding this to this PR. As far as I'm concerned, executing one or the other (or even both) is fine by me.
There was a problem hiding this comment.
Thanks for the feedbacks! Let me go forward with the immediate exit for now. I want to make it very explicit.
There was a problem hiding this comment.
Addressed in the last commit
main.go
Outdated
| existingFile = DeprecatedHelmfile | ||
| } | ||
|
|
||
| if specifiedPath == "" && existingFile != "" && existingDir != "" { |
There was a problem hiding this comment.
I believe that specifiedPath == "" is needed in order to allow the user to selectively run helmfile sync for the default helmfile.d or helmfile.yaml, and helmfile sync -f $path for a specific helmfile
There was a problem hiding this comment.
The logic looks correct after stepping through a bunch of options, but it's very complicated. I think I would move the DefaultHelmfile/DeprecatedHelmfile check as part of the first if else. I also think you can drop the specifiedPath=="" from this if statement as the first function should always set existingDir if specifiedPath is set or return.
And of course I could be missing something on my recommendations.
There was a problem hiding this comment.
Ah, so this block should be nested inside the } else if pathExists(DefaultHelmfileDirectory) && directoryExists(DefaultHelmfileDirectory) { block in the first if-else?
Sounds good! I'll address it asap. Thanks 🙏
|
Can I merge this? 😉 |
mumoshu
changed the title
|
@sstarcher Thanks for the review. I wonder if it's a bit more readable now :) |
|
Itching to use this!! |
|
Can we get a new release of |
|
@mumoshu could you explain a bit how to use So i have
Another question is about path to the values-files if |
|
It works for |
|
Also, we are not able to use Using any other labels also fail. Using explicit file (e.g. |
Resolves #151 #137