-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add daemon mode flag #161
base: main
Are you sure you want to change the base?
Add daemon mode flag #161
Conversation
cmd/spiffe-helper/main.go
Outdated
configFile := flag.String("config", "helper.conf", "<configFile> Configuration file path") | ||
exitWhenReady := flag.Bool("exitWhenReady", false, "Exit once the requested objects are retrieved") | ||
disableDaemonMode := flag.Bool("disable_daemon_mode", false, "Exit once the requested objects are retrieved") |
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.
Isn't the config daemon_mode
, not disable_daemon_mode
?
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.
Changed to daemon-mode
7bac373
to
e6e9234
Compare
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
configFile := flag.String("config", "helper.conf", "<configFile> Configuration file path") | ||
daemonModeFlag := flag.Bool(daemonModeFlagName, true, "Exit once the requested objects are retrieved") | ||
flag.Parse() |
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.
why to parse here? generally it is expected than flags are parsed in main so it is easy to found,
is it for testing?
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.
Yes for unit testing. See related tested here: https://github.com/spiffe/spiffe-helper/pull/161/files#diff-a7f12ddbf036810a09ab419309d276dae4b679371fc962c1ad5a63cd0cf790ccR400
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 can move back to main and have duplicate for tests if you think thats better.
return errors.New("at least one of the sets ('svid_file_name', 'svid_key_file_name', 'svid_bundle_file_name'), 'jwt_svids', or 'jwt_bundle_file_name' must be fully specified") | ||
} | ||
|
||
if x509EmptyCount != 0 && x509EmptyCount != 3 { | ||
return errors.New("all or none of 'svid_file_name', 'svid_key_file_name', 'svid_bundle_file_name' must be specified") | ||
if isFlagPassed(daemonModeFlagName) { |
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.
can you unify daemonModeFlag to a single value?
it must be calculated before getting here, so we can just set the value we get
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.
Can move to ParseConfig()
. Is that what you're thinking?
Signed-off-by: Faisal Memon <fymemon@yahoo.com> Co-authored-by: Marcos Yacob <marcos.yacob@hpe.com> Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Co-authored-by: Marcos Yacob <marcos.yacob@hpe.com> Signed-off-by: Faisal Memon <fymemon@yahoo.com>
daemon_mode
configurable to toggle running spiffe-helper as a daemon. When daemon mode is disabled x509 and/or jwts are downloaded and spiffe-helper exits.daemon-mode
cli flag to override config file setting and disable daemon mode.