Add variable for global flags to runlabel #2905
Conversation
QiWang19
changed the title
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
Initial impression is that this looks about right - just needs to pick up the rest of the local flags |
QiWang19
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
do I need to add tests for this? |
|
Tests are always appreciated! |
|
Test added |
|
/approved |
cmd/podman/runlabel.go
Outdated
| @@ -179,3 +180,66 @@ func runlabelCmd(c *cliconfig.RunlabelValues) error { | |||
|
|
|||
| return utils.ExecCmdWithStdStreams(stdIn, stdOut, stdErr, env, cmd[0], cmd[1:]...) | |||
| } | |||
|
|
|||
| func getGlobalOpts(c *cliconfig.RunlabelValues) string { | |||
There was a problem hiding this comment.
@mheon, should this live in a common or util file somewhere?
There was a problem hiding this comment.
I've moved it to package util
|
LGTM, just one question on a final resting place for one of the functions. |
cmd/podman/shared/funcs.go
Outdated
| @@ -60,6 +60,8 @@ func GenerateCommand(command, imageName, name string) ([]string, error) { | |||
| } | |||
| newCommand = append(newCommand, prog) | |||
|
|
|||
| fmt.Println(strings.Join(cmd, ",")) | |||
| @@ -324,3 +325,67 @@ func ParseInputTime(inputTime string) (time.Time, error) { | |||
| } | |||
| return time.Now().Add(-duration), nil | |||
| } | |||
|
|
|||
| // GetGlobalOpts checks all global flags and generates the command string | |||
| func GetGlobalOpts(c *cliconfig.RunlabelValues) string { | |||
There was a problem hiding this comment.
I wonder if we can somehow pull the names of the global flags straight out of Cobra - @baude you know if there's a way to do this?
There was a problem hiding this comment.
I found FlagSet.VisitAll() for all flags, not just for global flags...
There was a problem hiding this comment.
Hm. This could be painful if we add more global flags, but no helping that, I suppose.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon, QiWang19 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
☔ The latest upstream changes (presumably #3031) made this pull request unmergeable. Please resolve the merge conflicts. |
|
LGTM |
use $GLOBAL_OPTS to pass global flags to the runlabel command. Signed-off-by: Qi Wang <qiwan@redhat.com>
| // GetGlobalOpts checks all global flags and generates the command string | ||
| func GetGlobalOpts(c *cliconfig.RunlabelValues) string { | ||
| globalFlags := map[string]bool{ | ||
| "cgroup-manager": true, "cni-config-dir": true, "conmon": true, "default-mounts-file": true, |
There was a problem hiding this comment.
Separate PR, but could you move this to main_local.go
Add one line after each global flag gets added
rootCmd.PersistentFlags().StringVar(&MainGlobalOpts.CGroupManager, "cgroup-manager", "", "Cgroup manager to use (cgroupfs or systemd, default systemd)")
globalFlags["cgroup-manager"]=true
rootCmd.PersistentFlags().StringVar(&MainGlobalOpts.CpuProfile, "cpu-profile", "", "Path for the cpu profiling results")
globalFlags["cpu-profile"]=true
That way if a user adds a global flag, it is much more likely they will do the right thing.
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
close #2399
use $GLOBAL_OPTS to pass global flags to the runlabel command.
Signed-off-by: Qi Wang qiwan@redhat.com