sighupio / furyagent

Apache License 2.0
9 stars 2 forks source link

Fix furyagent version command and bump version. #11

Closed ralgozino closed 4 years ago

ralgozino commented 4 years ago

This PR fixes #10

angelbarrera92 commented 4 years ago

The same error happened in the printDefault command, which does not make sense. The printDefault command was unimplemented so, it's better to remove it (this is why i have pushed this change)

lnovara commented 4 years ago

Can't we just move the -config flag only in the subcommands where it makes sens to have it? Or is the PersistentPreRun the standard way to address this kind of issues?

ralgozino commented 4 years ago

Can't we just move the -config flag only in the subcommands where it makes sens to have it? Or is the PersistentPreRun the standard way to address this kind of issues?

The problem is not the flag, but the configuration file loading.

That was my intention at first but 99% of commands require the config loaded. version and help (that's "built-in") are the only ones that don't. In my opinion we can leave it like this, at least for the time being to solve this issue, a further global re-write is needed.

lnovara commented 4 years ago

I agree, it was more a curiosity of mine than a real issue. 😅

lnovara commented 4 years ago

@ralgozino can we handle the version package in furyagent in the same way as we do for furyctl? This way, we don't have to remember to manually bump the version when we release a new one.

Refs:

ralgozino commented 4 years ago

Yep! I agree, we should totally use the same approach that we are using for furyctl

EDIT: I've just pushed the changes to use ldflags for the version just like in furyctl. Please @angelbarrera92 take a look into it :)

angelbarrera92 commented 4 years ago

Yep! I agree, we should totally use the same approach that we are using for furyctl

EDIT: I've just pushed the changes to use ldflags for the version just like in furyctl. Please @angelbarrera92 take a look into it :)

The ldflags looks good to me! Same for the rest of the PR! Awesome work!