Skip to content
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

(#8) Support opt-out of os.Args #9

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

jeffmccune
Copy link
Contributor

Problem:
Cannot embed machine room as a cobra subcommand because fisk and cobra fight over os.Args

Solution:
Add a CustomArgs and CustomArgsEnabled machine room option to pass args through.

Result:
Machine room can run as a cobra subcommand:

cmd := &cobra.Command{
  Args:               cobra.ArbitraryArgs,
  DisableFlagParsing: true,
  RunE: func(cmd *cobra.Command, args []string) error {
    app, _ := mr.New(mr.Options{
      CustomArgsEnabled: true,
      CustomArgs:        args,
    })
    return app.Run(cmd.Context())
  },
}

Closes: #8

Problem:
Cannot embed machine room as a cobra subcommand because fisk and cobra
fight over os.Args

Solution:
Add a CustomArgs and CustomArgsEnabled machine room option to pass args
through.

Result:
Machine room can run as a cobra subcommand:

```go
cmd := &cobra.Command{
  Args:               cobra.ArbitraryArgs,
  DisableFlagParsing: true,
  RunE: func(cmd *cobra.Command, args []string) error {
    app, _ := mr.New(mr.Options{
      CustomArgsEnabled: true,
      CustomArgs:        args,
    })
    return app.Run(cmd.Context())
  },
}
```
@ripienaar ripienaar merged commit bdfb995 into choria-io:main Apr 16, 2024
1 check passed
@ripienaar
Copy link
Member

nice one, thanks

@@ -121,6 +123,10 @@ type Options struct {
// NoNetworkFacts disables built-in network interface facts gathering
NoNetworkFacts bool `json:"no_network_facts,omitempty"`

// os.Args opt-out https://github.com/choria-io/machine-room/issues/8
CustomArgsEnabled bool `json:"-"`
CustomArgs []string `json:"-"`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ripienaar Thanks for merging this, but I was wondering: Is the boolean unnecessary? Could I have just checked if CustomArgs is nil?

At least this is a bit more readable, so won't change it unless you want me to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you're right, checking len(CustomArgs)==0 would be better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool let me fix it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed 2 dependency updates - one also adding a new watcher

https://choria.io/docs/autoagents/watcher_reference/#expression-watcher

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna check nil, len and cap are 0 for a nil slice.

package main

import "fmt"

func main() {
	var args []string
	p("args", args)
	fmt.Println()
	p("args2", []string{})
}

func p(name string, args []string) {
	if args == nil {
		fmt.Println(name, "is nil")
	} else {
		fmt.Println(name, "is NOT nil")
	}
	fmt.Printf("%s: %+v\n", name, args)
	fmt.Printf("len(%s): %+v\n", name, len(args))
	fmt.Printf("cap(%s): %+v\n", name, cap(args))
}
❯ go run foo.go
args is nil
args: []
len(args): 0
cap(args): 0

args2 is NOT nil
args2: []
len(args2): 0
cap(args2): 0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

len()==0 is enough. Nil slice is 0 len.

Copy link
Contributor Author

@jeffmccune jeffmccune Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it as a nil check because I think we want to disambiguate between the case where the user passes in args of length 0 and the case where the user didn't pass in any args at all. I think we should only use the override args if they're initialized by the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support cobra integration
2 participants