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

Adding Chdir option like terraform #12264

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 67 additions & 1 deletion main.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"math/rand"
"os"
"runtime"
"strings"
"sync"
"syscall"
"time"
Expand Down Expand Up @@ -123,6 +124,53 @@ func realMain() int {
return 0
}

func extractChdirOption(args []string) (string, []string, error) {
if len(args) == 0 {
return "", args, nil
}

const argName = "-chdir"
const argPrefix = argName + "="
var argValue string
var argPos int

for i, arg := range args {
if !strings.HasPrefix(arg, "-") {
Copy link
Contributor

Choose a reason for hiding this comment

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

With respect to the other global arguments we support, AFAICT, they can be specified anywhere in the command-line.
While Terraform seems to enforce the location of the argument, I would think that in our case, in order to remain consistent with the rest of the options, we should allow it to be specified anywhere.

// Because the chdir option is a subcommand-agnostic one, we require
// it to appear before any subcommand argument, so if we find a
// non-option before we find -chdir then we are finished.
break
}
if arg == argName || arg == argPrefix {
return "", args, fmt.Errorf("must include an equals sign followed by a directory path, like -chdir=example")
Copy link
Contributor

Choose a reason for hiding this comment

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

Again with respect to the other arguments we support, they can be specified like what is printed out here or with variations.
Typically, we can specify options with either one or two -, with the exception of the -machine-readable option, which is hard-coded with one - prefix.
The = for the value is also not strictly required, options can be specified in two parts, with the value being the next argument, if unspecified as part of the introduction.
For consistency with the other arguments, I would prefer this keeps the same convention, but this does complexify the code.

}
if strings.HasPrefix(arg, argPrefix) {
argPos = i
argValue = arg[len(argPrefix):]
Copy link

Choose a reason for hiding this comment

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

Suggested change
argValue = arg[len(argPrefix):]
argValue = arg[len(argPrefix):]
break

Copy link

Choose a reason for hiding this comment

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

In the event there is other args after -chdir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! Added this change to PR. Please let me know if there's anything else I should change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Bit late to the party, but as a counterpoint, I would suggest maybe going through all the arguments maybe, so that if someone defines --chdir twice, we can error.
That, or we could arbitrarily decide to use the last one as truth value, but this may end-up confusing imho, so I would think erroring might be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

That being said, I see that for the other global argument we support (-machine-readable), we don't do this, so I presume the subcommand will take care of erroring in this case.
This would make for a good argument in favour of not checking for this use-case here, or maybe adopting a similar duplication-check for both.

break
}
}

// When we fall out here, we'll have populated argValue with a non-empty
// string if the -chdir=... option was present and valid, or left it
// empty if it wasn't present.
if argValue == "" {
return "", args, nil
}

// If we did find the option then we'll need to produce a new args that
// doesn't include it anymore.
if argPos == 0 {
// Easy case: we can just slice off the front
return argValue, args[1:], nil
}
// Otherwise we need to construct a new array and copy to it.
newArgs := make([]string, len(args)-1)
copy(newArgs, args[:argPos])
copy(newArgs[argPos:], args[argPos+1:])
return argValue, newArgs, nil
}

// wrappedMain is called only when we're wrapped by panicwrap and
// returns the exit status to exit with.
func wrappedMain() int {
Expand Down Expand Up @@ -188,9 +236,27 @@ func wrappedMain() int {
}
log.Printf("[INFO] Setting cache directory: %s", cacheDir)

// Below is same as terraform's chdir setup
// https://github.com/hashicorp/terraform/blob/main/main.go
// The arguments can begin with a -chdir option to ask Terraform to switch
// to a different working directory for the rest of its work. If that
// option is present then extractChdirOption returns a trimmed args with that option removed.
overrideWd, args, err := extractChdirOption(os.Args[1:])
if err != nil {
fmt.Fprintf(os.Stdout, "%s Invalid -chdir option: \n\n%s\n", ErrorPrefix, err)
return 1
}
if overrideWd != "" {
err := os.Chdir(overrideWd)
if err != nil {
fmt.Fprintf(os.Stdout, "%s Error handling -chdir option: \n\n%s\n", ErrorPrefix, err)
return 1
}
}

// Determine if we're in machine-readable mode by mucking around with
// the arguments...
args, machineReadable := extractMachineReadable(os.Args[1:])
args, machineReadable := extractMachineReadable(args)

defer packer.CleanupClients()

Expand Down
53 changes: 53 additions & 0 deletions main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,59 @@ func TestExcludeHelpFunc(t *testing.T) {
}
}

func TestExtractChdir(t *testing.T) {
cases := []struct {
desc, overrideWd string
args, expected []string
err error
}{
{
desc: "TestHappyPath",
args: []string{"-chdir=example", "foo", "bar"},
expected: []string{"foo", "bar"},
overrideWd: "example",
err: nil,
},
{
desc: "TestEmptyArgs",
args: []string{},
expected: []string{},
overrideWd: "",
err: nil,
},
{
desc: "TestNoChdirArg",
args: []string{"foo", "bar"},
expected: []string{"foo", "bar"},
overrideWd: "",
err: nil,
},
{
desc: "TestChdirNotFirst",
args: []string{"foo", "-chdir=example"},
expected: []string{"foo", "-chdir=example"},
overrideWd: "",
err: nil,
},
}

for _, tc := range cases {
overrideWd, args, err := extractChdirOption(tc.args)
if overrideWd != tc.overrideWd {
t.Fatalf("%s: bad overrideWd, expected: %s got: %s for args: %#v",
tc.desc, tc.overrideWd, overrideWd, tc.args)
}

if !reflect.DeepEqual(args, tc.expected) {
t.Fatalf("%s: bad result args, expected: %#v, got: %#v for args: %#v", tc.desc, tc.expected, args, tc.args)
}

if err != tc.err {
t.Fatalf("%s: bad err, expected: %s, got: %s for args: %#v", tc.desc, tc.err, err, tc.args)
}
}
}

func TestExtractMachineReadable(t *testing.T) {
var args, expected, result []string
var mr bool
Expand Down