allow passing whole directory as -f argument #166
Conversation
|
@kadel, thank you for the pull request! We'll ping some people to review your PR. @containscafeine, please review this. |
kadel
force-pushed
the
readFromDir
branch
7 times, most recently
from
93ca04d
to
3e3e3f7
Compare
kadel
changed the title
|
passing in a directory with no YAML files does not fail and exits silently, let's error out here? - $ kedge generate -f examples/
$
$ echo $?
0 |
good catch @containscafeine I've added error |
pkg/cmd/util.go
Outdated
|
|
||
| yamlFiles, err := filepath.Glob(filepath.Join(path, "*.yaml")) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "can't list *.yml files in %s", path) |
|
@kadel apart from that spell mistake rest LGTM! |
pkg/cmd/util.go
Outdated
|
|
||
| // GetAllYMLFiles if path in argument is directory get all *.yml and *.yaml files | ||
| // in that directory. If path is file just add it to output list as it is. | ||
| func GetAllYMLFiles(paths []string) ([]string, error) { |
There was a problem hiding this comment.
nit: rename to GetAllYAMLFiles?
pkg/cmd/util.go
Outdated
| @@ -56,3 +58,40 @@ func getApplicationsFromFiles(files []string) ([]inputData, error) { | |||
| } | |||
| return appData, nil | |||
| } | |||
|
|
|||
| // GetAllYMLFiles if path in argument is directory get all *.yml and *.yaml files | |||
There was a problem hiding this comment.
Do we need the string "GetAllYMLFiles" in the beginning?
There was a problem hiding this comment.
yes ;-)
Why would you want to remove it? In favor of what?
There was a problem hiding this comment.
Haha, I mean why do we want the function name in the starting of the comment?
pkg/cmd/util.go
Outdated
| } | ||
| files = append(files, ymlFiles...) | ||
|
|
||
| yamlFiles, err := filepath.Glob(filepath.Join(path, "*.yaml")) |
There was a problem hiding this comment.
We are having to run filepath.Glob twice, once for yaml and one for yml. Can we use some pattern matching to make it one?
I tried ls *.y{a,}ml for the ls command, and it worked there, can we have something similar here?
There was a problem hiding this comment.
Patern language for Glob is descibed in https://golang.org/pkg/path/filepath/#Match
Haven't found a way how to do it in one query :-(
pkg/cmd/util.go
Outdated
| } | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
which is fine, but we are not following this style anywhere else in the code (or at least I could not find it) :( So to keep with the code style, does it make sense to keep it?
There was a problem hiding this comment.
For me, it makes the code more readable, but whatever, I've removed the lines. I don't want to argue about empty lines.
pkg/cmd/util.go
Outdated
| if len(files) == 0 { | ||
| return nil, fmt.Errorf("no *.yml or *.yaml files were found") | ||
| } | ||
|
|
| func Generate(files []string) error { | ||
| func Generate(paths []string) error { | ||
|
|
||
| files, err := GetAllYAMLFiles(paths) |
There was a problem hiding this comment.
@kadel instead of changing functions here, and getting all YAML files here, I think it makes more sense to do this in cmd/{apply,create,delete,generate}.go, similar to the way if err := ifFilesPassed(InputFiles); err != nil { is being done in those files.
We can create one validateFiles function in cmd/util.go which calls ifFilesPassed and GetAllYAMLFiles since these are common in all of cmd/{apply,create,delete,generate}.go files, and just call validateFiles in all of them.
It will make this behavior more uniform instead of adding in func Generate used by only generate.go and func ExecuteKubectl used by {apply,create,delete}.go.
WDYT?
There was a problem hiding this comment.
it was done like this in the first place, but then I changed that, to this.
It's closer to application logic and I think that in cmd/* should be mostly the code that handles argument parsing and validation.
It's also better to have it inside ExecuteKubectl and in Generate as it can't happen that someone will forget to call when using Generate/ExecuteKubectl
if dir is provided, use all *.yml and *.yaml files in that directory
if dir is provided, use all *.yml files in that directory
fixes #155
TODO:
GetAllYMLFilesfromExecuteKubectl?