-
Notifications
You must be signed in to change notification settings - Fork 564
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
Ordered helmfile(s) from a directory #160
Conversation
986533d
to
9a9a30b
Compare
main.go
Outdated
dir = DefaultHelmfileDirectory | ||
} | ||
|
||
if dir != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is clearer
if dir != "" {
...
else if fileExists(DefaultHelmfileDirectory) && directoryExists(DefaultHelmfileDirectory) {
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it look clearer now?
main.go
Outdated
return files, nil | ||
} | ||
|
||
if fileExists(DefaultHelmfile) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in the last commit
main.go
Outdated
existingFile = DeprecatedHelmfile | ||
} | ||
|
||
if specifiedPath == "" && existingFile != "" && existingDir != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? 😉 |
@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